Feature: pulling consumer subscriber #587

astelmashenko · 2024-07-31T13:42:48Z

Proposed Changes

introducing PullConsumer in parallel with legacy PushConsumer
switching e2e tests to test jetstream dispatcher

knative-prow · 2024-07-31T13:42:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: astelmashenko

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~~ [astelmashenko]

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

codecov · 2024-08-01T15:46:26Z

Codecov Report

Attention: Patch coverage is 14.21801% with 362 lines in your changes missing coverage. Please review.

Project coverage is 41.12%. Comparing base (eb80414) to head (7e35dba).
Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/channel/jetstream/dispatcher/pull_consumer.go	1.04%	188 Missing and 1 partial ⚠️
pkg/channel/jetstream/dispatcher/push_consumer.go	6.41%	73 Missing ⚠️
pkg/channel/jetstream/dispatcher/defaults.go	24.48%	35 Missing and 2 partials ⚠️
pkg/channel/jetstream/dispatcher/dispatcher.go	50.87%	25 Missing and 3 partials ⚠️
pkg/channel/jetstream/utils/consumerconfig.go	23.07%	20 Missing ⚠️
pkg/tracing/tracing.go	0.00%	11 Missing ⚠️
...channel/jetstream/dispatcher/message_dispatcher.go	55.55%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #587      +/-   ##
==========================================
- Coverage   43.57%   41.12%   -2.45%     
==========================================
  Files          61       62       +1     
  Lines        2678     3664     +986     
==========================================
+ Hits         1167     1507     +340     
- Misses       1430     2072     +642     
- Partials       81       85       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…plate

… fetchmaxwait type, correctly closing pull consumer

astelmashenko · 2024-08-05T14:17:00Z

/assign @Cali0707

astelmashenko · 2024-08-05T14:17:18Z

@Cali0707 , could you please review.

Cali0707

I'm not very familiar with the NATS-specific stuff happening here, but I did my best to do a first pass review @astelmashenko

Cali0707 · 2024-09-11T14:51:37Z

pkg/channel/jetstream/dispatcher/pull_consumer.go

+	// you have more time for resources to spin up.
+	//
+	// If you have limited resources, you may want to reduce the number of goroutines.
+	NumBatchGoRoutines = 32


Maybe I'm missing something, but as far as I can tell we aren't using this value anywhere?

yeah, you are right, I was planning to introduce later worker pools per subscriber.

should we remove it for now then?

agree. removed.

pkg/channel/jetstream/dispatcher/push_consumer.go

pkg/channel/jetstream/dispatcher/pull_consumer.go

Cali0707

I'm not an expert on any of the NATS parts, but this looks fine to me 👍

/lgtm
/hold for any other reviews

astelmashenko · 2024-10-04T11:56:08Z

@Cali0707 , hey thanks for reviewing it, if you have somebody in mind who can review it then please mention him here, otherwise I do not know why hold, the PR is two months old, enough time to check.
Basically it will not change how it's working by default unless you intentionally pass configuration param to use pulling consumers.

I'm waiting this PR to continue working on the eventing-nats:

remove dweprecated (and not supported) nats-streaming
introduce a logic to seamlessly switch from deprecated push-consumers

Cali0707 · 2024-10-04T21:16:09Z

Sure @astelmashenko let's merge it then
/unhold

astelmashenko · 2024-10-07T11:15:36Z

Hey, @Cali0707 , could we skip codecov for this PR? In the PR I do:

refactor code to support push/pull based consumers (push based is default and without changes)
changed github actions to run tests against jetstream implementation (was for legacy nats-streaming)

Because of refactorings and new code there is such codecov that does not allow to merge.

dsimansk · 2024-10-21T13:16:03Z

/override codecov/project

knative-prow · 2024-10-21T13:16:08Z

@dsimansk: Overrode contexts on behalf of dsimansk: codecov/project

In response to this:

/override codecov/project

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

dsimansk · 2024-10-21T13:16:14Z

/override codecov/patch

knative-prow · 2024-10-21T13:16:17Z

@dsimansk: Overrode contexts on behalf of dsimansk: codecov/patch

In response to this:

/override codecov/patch

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

dsimansk · 2024-10-21T13:18:31Z

/lgtm

astelmashenko added 4 commits July 30, 2024 13:21

added (unused for now) pulling consumer implementation

3f7fd3f

added (unused for now) pulling consumer implementation

08806e1

Implemented subscribe for pull and push subs

089fd69

switching e2e tests from nats-streaming to jetstream

5f60357

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

knative-prow bot requested review from Cali0707 and dan-j July 31, 2024 13:42

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 31, 2024

fixed nats message published in structured way

209c0cb

astelmashenko added 5 commits August 2, 2024 11:49

fixed e2e tests

4b0723a

fixed codestyle and lint errors

ece918f

fixed pull consumer start, added pull consumer params into channeltem…

4c5aadc

…plate

implemented update pull subscription, fixed closing consumer, changed…

61ee864

… fetchmaxwait type, correctly closing pull consumer

fixed fetchmaxwait pull consumer

aabb79e

knative-prow bot assigned Cali0707 Aug 5, 2024

astelmashenko marked this pull request as ready for review August 5, 2024 14:17

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 5, 2024

knative-prow bot requested a review from creydr August 5, 2024 14:17

Cali0707 reviewed Sep 11, 2024

View reviewed changes

removed unused const

56921ee

Cali0707 reviewed Oct 3, 2024

View reviewed changes

knative-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Oct 3, 2024

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 4, 2024

removed redundant internal message.go

7e35dba

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2024

knative-prow bot assigned dsimansk Oct 21, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2024

knative-prow bot merged commit c76507b into knative-extensions:main Oct 21, 2024
21 of 23 checks passed

astelmashenko mentioned this pull request Oct 21, 2024

cloudevent are published in structured mode now #599

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: pulling consumer subscriber #587

Feature: pulling consumer subscriber #587

astelmashenko commented Jul 31, 2024 •

edited

Loading

knative-prow bot commented Jul 31, 2024

codecov bot commented Aug 1, 2024 •

edited

Loading

astelmashenko commented Aug 5, 2024

astelmashenko commented Aug 5, 2024

Cali0707 left a comment

Cali0707 Sep 11, 2024

astelmashenko Sep 12, 2024

Cali0707 Sep 16, 2024

astelmashenko Sep 17, 2024

Cali0707 left a comment

astelmashenko commented Oct 4, 2024 •

edited

Loading

Cali0707 commented Oct 4, 2024

astelmashenko commented Oct 7, 2024

dsimansk commented Oct 21, 2024

knative-prow bot commented Oct 21, 2024

dsimansk commented Oct 21, 2024

knative-prow bot commented Oct 21, 2024

dsimansk commented Oct 21, 2024

Feature: pulling consumer subscriber #587

Feature: pulling consumer subscriber #587

Conversation

astelmashenko commented Jul 31, 2024 • edited Loading

Proposed Changes

knative-prow bot commented Jul 31, 2024

codecov bot commented Aug 1, 2024 • edited Loading

Codecov Report

astelmashenko commented Aug 5, 2024

astelmashenko commented Aug 5, 2024

Cali0707 left a comment

Choose a reason for hiding this comment

Cali0707 Sep 11, 2024

Choose a reason for hiding this comment

astelmashenko Sep 12, 2024

Choose a reason for hiding this comment

Cali0707 Sep 16, 2024

Choose a reason for hiding this comment

astelmashenko Sep 17, 2024

Choose a reason for hiding this comment

Cali0707 left a comment

Choose a reason for hiding this comment

astelmashenko commented Oct 4, 2024 • edited Loading

Cali0707 commented Oct 4, 2024

astelmashenko commented Oct 7, 2024

dsimansk commented Oct 21, 2024

knative-prow bot commented Oct 21, 2024

dsimansk commented Oct 21, 2024

knative-prow bot commented Oct 21, 2024

dsimansk commented Oct 21, 2024

astelmashenko commented Jul 31, 2024 •

edited

Loading

codecov bot commented Aug 1, 2024 •

edited

Loading

astelmashenko commented Oct 4, 2024 •

edited

Loading