add Channel NatsJetStreamChannel to support JetStream #182

zhaojizhuang · 2021-07-07T09:34:58Z

Working in progress...

Part 1 of #145

Proposed Changes

add NatsJetStreamChannel support JetStream

Re-optimized the directory structure and file name to support streaming and jetstream
Add crds and controller for jetstream
Add dispatcher for jetstream

Release Note

Docs

zhaojizhuang · 2021-07-07T09:35:06Z

/wip

zhaojizhuang · 2021-07-07T09:38:58Z

/hold
work in progress

codecov · 2021-07-08T03:55:00Z

Codecov Report

Merging #182 (058c044) into main (2c87814) will decrease coverage by 3.82%.
The diff coverage is 64.15%.

@@            Coverage Diff             @@
##             main     #182      +/-   ##
==========================================
- Coverage   77.25%   73.43%   -3.83%     
==========================================
  Files          10       17       +7     
  Lines         299      399     +100     
==========================================
+ Hits          231      293      +62     
- Misses         53       90      +37     
- Partials       15       16       +1

Impacted Files	Coverage Δ
...saging/v1alpha1/nats_jetstream_channel_defaults.go	`0.00% <0.00%> (ø)`
pkg/apis/messaging/v1alpha1/register.go	`0.00% <0.00%> (ø)`
pkg/natsutil/jetstreamutil.go	`0.00% <0.00%> (ø)`
pkg/natsutil/stanutil.go	`71.42% <ø> (ø)`
pkg/reconciler/controller/natss/natsschannel.go	`79.62% <ø> (ø)`
...g/reconciler/controller/natss/resources/service.go	`100.00% <ø> (ø)`
...ging/v1alpha1/nats_jetstream_channel_validation.go	`57.14% <57.14%> (ø)`
...pis/messaging/v1alpha1/nats_jetstream_lifecycle.go	`95.45% <95.45%> (ø)`
...kg/apis/messaging/v1alpha1/nats_jetstream_types.go	`100.00% <100.00%> (ø)`
...conciler/controller/jetstream/resources/service.go	`100.00% <100.00%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c87814...058c044. Read the comment docs.

zhaojizhuang · 2021-07-08T06:11:04Z

/unhold

zhaojizhuang · 2021-07-08T06:23:11Z

/assign @devguyio @matzew

zhaojizhuang · 2021-08-09T07:58:23Z

ping @vaikas @devguyio

vaikas · 2021-08-09T09:18:33Z

config/200-jsm-addressable-resolver-clusterrole.yaml

@@ -0,0 +1,32 @@
+# Copyright 2019 The Knative Authors


nit: Some of these files have the wrong copyright year. Copy/paste leftover?

vaikas · 2021-08-09T09:20:02Z

config/200-jsm-dispatcher-service.yaml

+  name: jetstream-ch-dispatcher
+  namespace: knative-eventing
+  labels:
+    nats.eventing.knative.dev/release: devel


I think this should be natsjsm?
Just like here:
https://github.com/knative-sandbox/eventing-natss/pull/182/files#diff-a9bd98b0e5248a4625de7f3a112db792e398a7fd49924ac9bd40bec7f0a788fdR20

@vaikas Or we should be nats（include natss and natsjsm）? We release both to together. What do you think about?

vaikas · 2021-08-09T09:20:35Z

config/200-jsm-serviceaccount.yaml

+  name: nats-jsm-ch-controller
+  namespace: knative-eventing
+  labels:
+    nats.eventing.knative.dev/release: devel


Same here?
Match:
https://github.com/knative-sandbox/eventing-natss/pull/182/files#diff-a9bd98b0e5248a4625de7f3a112db792e398a7fd49924ac9bd40bec7f0a788fdR20

vaikas · 2021-08-09T09:20:49Z

config/200-jsm-serviceaccount.yaml

+  name: nats-jsm-ch-dispatcher
+  namespace: knative-eventing
+  labels:
+    nats.eventing.knative.dev/release: devel


And here, natsjsm?

vaikas · 2021-08-09T09:21:06Z

config/201-jsm-clusterrolebinding.yaml

@@ -0,0 +1,44 @@
+# Copyright 2019 The Knative Authors


vaikas · 2021-08-09T09:21:41Z

config/302-jetstream-channel.yaml

+metadata:
+  name: natsjetstreamchannels.messaging.knative.dev
+  labels:
+    nats.eventing.knative.dev/release: devel


vaikas · 2021-08-09T09:22:56Z

config/501-jetstream-controller.yaml

+  name: jetstream-ch-controller
+  namespace: knative-eventing
+  labels:
+    nats.eventing.knative.dev/release: devel


same as above

vaikas · 2021-08-09T09:23:18Z

config/501-jetstream-dispatcher.yaml

+  name: jetstream-ch-dispatcher
+  namespace: knative-eventing
+  labels:
+    nats.eventing.knative.dev/release: devel


vaikas · 2021-08-09T09:24:44Z

hack/release.sh

-    echo "Tagged release, updating release labels to natss.eventing.knative.dev/release: \"${TAG}\""
-    LABEL_YAML_CMD=(sed -e "s|natss.eventing.knative.dev/release: devel|natss.eventing.knative.dev/release: \"${TAG}\"|")
+    echo "Tagged release, updating release labels to nats.eventing.knative.dev/release: \"${TAG}\""
+    LABEL_YAML_CMD=(sed -e "s|nats.eventing.knative.dev/release: devel|nats.eventing.knative.dev/release: \"${TAG}\"|")


You probably need to add something about replacing the natsjsm.eventing release labels too?

vaikas · 2021-08-09T09:34:40Z

pkg/reconciler/controller/jetstream/controller.go

+		FilterFunc: filterFunc,
+		Handler:    controller.HandleAll(grCh),
+	})
+	serviceInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{


Maybe I'm wrong. But this sets up the (what looks to be a single dispatcher for all the channels?) and that causes a global resync, which I think makes sense since if the dispatcher is used by all the channels, they all need to be reconciled. However, there's another Service thats created here (obvs if it doesn't exist):
https://github.com/knative-sandbox/eventing-natss/pull/182/files#diff-a9bd98b0e5248a4625de7f3a112db792e398a7fd49924ac9bd40bec7f0a788fdR20

For that I'd expect us to set up a tracker so that only one channel get's reconciled when that Service changes.

zhaojizhuang · 2021-08-09T12:42:01Z

Updated, add service eventhandler

vaikas · 2021-08-10T08:59:59Z

/lgtm
/approve
/hold

Looks like a great start, I reckon we should get this in and continue as per the PR description, this is only the first part. I added a hold since today is a release day and seems like a major change that we wouldn't want to go in at the last minute (esp, since there are no e2e tests (future work as per the issue)). Feel free to remove the hold once the release is cut.

knative-prow-robot · 2021-08-10T09:00:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vaikas, zhaojizhuang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [zhaojizhuang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

zhaojizhuang · 2021-08-10T09:45:42Z

/lgtm
/approve
/hold

Looks like a great start, I reckon we should get this in and continue as per the PR description, this is only the first part. I added a hold since today is a release day and seems like a major change that we wouldn't want to go in at the last minute (esp, since there are no e2e tests (future work as per the issue)). Feel free to remove the hold once the release is cut.

@vaikas OK, thank you. It's ok to aprove after release day. I have already tracked the things to be done in the issue task #145

vaikas · 2021-08-10T13:53:53Z

@zhaojizhuang it's already been approved so you can just remove the hold label after the release has been cut, you can do this yourself (or should be able to :) ) so just do it at will.

zhaojizhuang · 2021-08-11T10:03:43Z

/unhold

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 7, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2021

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 7, 2021

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2021

zhaojizhuang changed the title ~~support JetStream~~ [WIP]support JetStream Jul 7, 2021

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 7, 2021

zhaojizhuang force-pushed the rename branch from fca433c to 3e23249 Compare July 8, 2021 03:34

zhaojizhuang changed the title ~~[WIP]support JetStream~~ [WIP]support JetStream ,Part 1 of #145 Jul 8, 2021

zhaojizhuang changed the title ~~[WIP]support JetStream ,Part 1 of #145~~ [WIP]support JetStream ,Part 1 Jul 8, 2021

zhaojizhuang force-pushed the rename branch from 3e23249 to 423b285 Compare July 8, 2021 03:45

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 8, 2021

zhaojizhuang changed the title ~~[WIP]support JetStream ,Part 1~~ [WIP]support JetStream ,Part 1 ，Re-optimized the directory structure and file name Jul 8, 2021

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 8, 2021

zhaojizhuang changed the title ~~[WIP]support JetStream ,Part 1 ，Re-optimized the directory structure and file name~~ support JetStream ,Part 1 ，Re-optimized the directory structure and file name Jul 8, 2021

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 8, 2021

knative-prow-robot assigned devguyio and matzew Jul 8, 2021

zhaojizhuang changed the title ~~support JetStream ,Part 1 ，Re-optimized the directory structure and file name~~ add Channel NatsJetStreamChannel to support JetStream Jul 9, 2021

zhaojizhuang force-pushed the rename branch from 423b285 to 40edc8c Compare July 9, 2021 08:55

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 9, 2021

zhaojizhuang force-pushed the rename branch 2 times, most recently from 469278c to 58cc8bd Compare July 9, 2021 09:10

zhaojizhuang requested a review from vaikas July 12, 2021 02:46

zhaojizhuang added 14 commits August 9, 2021 10:59

add nats jetstream cr

163fdee

update hack/update-codege.sh

a89194b

hack/update-codegen.sh

d4b1c2a

add nats go mod, go mod vendor

e3febad

add jetstream dispatcher and controller

6eeba7c

add jetstream and controller config

0e2f5db

hack update codegen

5745c20

add jet stream issue

962da30

support eventing-nats

8e5ef28

fix ut and update codegen

21962aa

update codegen

9bf2918

fix e2e

d21556c

update ENV

9d5a16f

rebase main

0d947af

zhaojizhuang force-pushed the rename branch from 805c5a1 to 0d947af Compare August 9, 2021 06:59

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2021

vaikas reviewed Aug 9, 2021

View reviewed changes

change label to nats.xxx

058c044

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 10, 2021

knative-prow-robot assigned vaikas Aug 10, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2021

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 11, 2021

knative-prow-robot merged commit 4080495 into knative-extensions:main Aug 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add Channel NatsJetStreamChannel to support JetStream #182

add Channel NatsJetStreamChannel to support JetStream #182

zhaojizhuang commented Jul 7, 2021 •

edited

Loading

zhaojizhuang commented Jul 7, 2021

zhaojizhuang commented Jul 7, 2021 •

edited

Loading

codecov bot commented Jul 8, 2021 •

edited

Loading

zhaojizhuang commented Jul 8, 2021

zhaojizhuang commented Jul 8, 2021

zhaojizhuang commented Aug 9, 2021

vaikas Aug 9, 2021

vaikas Aug 9, 2021

zhaojizhuang Aug 9, 2021 •

edited

Loading

vaikas Aug 9, 2021

vaikas Aug 9, 2021

vaikas Aug 9, 2021

vaikas Aug 9, 2021

zhaojizhuang Aug 9, 2021

vaikas Aug 9, 2021

zhaojizhuang Aug 9, 2021 •

edited

Loading

vaikas Aug 9, 2021

zhaojizhuang Aug 9, 2021

vaikas Aug 9, 2021

vaikas Aug 9, 2021

zhaojizhuang commented Aug 9, 2021

vaikas commented Aug 10, 2021

knative-prow-robot commented Aug 10, 2021

zhaojizhuang commented Aug 10, 2021 •

edited

Loading

vaikas commented Aug 10, 2021

zhaojizhuang commented Aug 11, 2021

add Channel NatsJetStreamChannel to support JetStream #182

add Channel NatsJetStreamChannel to support JetStream #182

Conversation

zhaojizhuang commented Jul 7, 2021 • edited Loading

Working in progress...

Proposed Changes

zhaojizhuang commented Jul 7, 2021

zhaojizhuang commented Jul 7, 2021 • edited Loading

codecov bot commented Jul 8, 2021 • edited Loading

Codecov Report

zhaojizhuang commented Jul 8, 2021

zhaojizhuang commented Jul 8, 2021

zhaojizhuang commented Aug 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaojizhuang Aug 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaojizhuang Aug 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaojizhuang commented Aug 9, 2021

vaikas commented Aug 10, 2021

knative-prow-robot commented Aug 10, 2021

zhaojizhuang commented Aug 10, 2021 • edited Loading

vaikas commented Aug 10, 2021

zhaojizhuang commented Aug 11, 2021

zhaojizhuang commented Jul 7, 2021 •

edited

Loading

zhaojizhuang commented Jul 7, 2021 •

edited

Loading

codecov bot commented Jul 8, 2021 •

edited

Loading

zhaojizhuang Aug 9, 2021 •

edited

Loading

zhaojizhuang Aug 9, 2021 •

edited

Loading

zhaojizhuang commented Aug 10, 2021 •

edited

Loading