Implemented delivery order label

[slinkydeveloper]

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

Part of #589

Proposed Changes

• 🎁 Implemented delivery order label
• 🐛 Modified default value for delivery order in contract
• 🗑️ Removed temporarily the usage of upstream features, since they're unstable and causes update issues

Release Note

Now eventing-kafka-broker implements ordered delivery. You can choose between ordered and unordered delivery through the label kafka.eventing.knative.dev/delivery.order in your triggers. Check out the docs for more details

Docs

Coming soon in another PR to docs repo

[slinkydeveloper]

I'm going to implement a basic e2e test, although I'm not sure how far I can go with testing this feature in a e2e manner

[codecov]

Codecov Report

Merging #701 (082dcb9) into main (1258e97) will decrease coverage by 0.15%.
The diff coverage is 76.92%.

@@             Coverage Diff              @@
##               main     #701      +/-   ##
============================================
- Coverage     76.78%   76.63%   -0.16%     
+ Complexity      441      440       -1     
============================================
  Files            77       77              
  Lines          2908     2923      +15     
  Branches        133      133              
============================================
+ Hits           2233     2240       +7     
- Misses          525      532       +7     
- Partials        150      151       +1     

Flag	Coverage Δ	Complexity Δ
java-unittests	80.37% <ø> (-0.25%)	0.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
control-plane/pkg/reconciler/trigger/trigger.go	71.42% <76.92%> (+0.22%)	0.00 <0.00> (ø)
...dispatcher/consumer/impl/OrderedOffsetManager.java	79.31% <0.00%> (-20.69%)	9.00% <0.00%> (-2.00%)
...r/dispatcher/http/HttpConsumerVerticleFactory.java	85.26% <0.00%> (ø)	24.00% <0.00%> (ø%)
...spatcher/consumer/impl/UnorderedOffsetManager.java	100.00% <0.00%> (+2.40%)	13.00% <0.00%> (+1.00%)

---

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 1258e97...082dcb9. Read the comment docs.

[slinkydeveloper]

/retest

[slinkydeveloper]

Because I would love to use reconciler-test for the e2e, I've opened this one #703

[pierDipi]

/this-is-fine 😍

[pierDipi]

If you want we can have a status condition to indicate the DeliveryOrder configured for a Trigger since it's easy to make a typo and then the DeliveryOrder configured is the default one rather than the one users wanted. (A separate PR is fine too)

[pierDipi]

/kind enhancement

[slinkydeveloper]

If you want we can have a status condition to indicate the DeliveryOrder configured for a Trigger since it's easy to make a typo and then the DeliveryOrder configured is the default one rather than the one users wanted. (A separate PR is fine too)

I don't see the point TBH, there's no need to tell the user the controller read the contract properly IMO

[pierDipi]

I mean an annotation is weak compared to a key in the spec because we don't have a way to validate that the key is correct for that reason a status condition would be nice to have IMO.

[slinkydeveloper]

@pierDipi can we tackle the status issue in another PR? From my understanding (I might be wrong here) we need to setup some alternate condition set, like done in receiver_condition_set.go, right? If that's the case, i think it deserves a separate PR

[slinkydeveloper]

Rebased on top of #730 for testing, we need to merge the #730 first

[slinkydeveloper]

I've removed broker_test.go, since upstream movement around features is unstable now...

For me we're ready to go
/unhold

[slinkydeveloper]

/hold

one sec, maybe it's better to use a label (that's what other people does 😄 )

[slinkydeveloper]

/unhold

Ok now we use labels

[pierDipi]

I see no benefits in using a label here, basically, the difference is that using a label I can query all objects with that label without doing a full scan or building a custom index whereas an annotation requires a full scan or a custom index.

For example, we use a label in our triggers to get fast access to all triggers associated with a broker.
https://github.com/knative-sandbox/eventing-kafka-broker/blob/1258e973843974a5a7fc42aff651cab4ed2a3f56/control-plane/pkg/reconciler/trigger/controller.go#L141-L157

In this case, a query like "give me all ordered trigger" seems pretty useless.

Of course, the benefit of using a label comes with additional storage, memory, and compute costs, so my preference would be to use an annotation since this value is only useful in a single reconcile cycle.

[slinkydeveloper]

@pierDipi the reason i went for a label is to align with kafka source: https://knative.dev/docs/eventing/samples/kafka/source/#optional-specify-the-key-deserializer

But yeah i was checking in serving and they use annotations too for "special"/"experimental" features, so i'll revert and go for the annotation

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-knative-sandbox-eventing-kafka-broker-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
control-plane/pkg/reconciler/trigger/trigger.go	85.6%	85.1%	-0.5

[pierDipi]

I wonder how this works given the issue on the partitionkey on Slack 🤔

[pierDipi]

Unhold when you're ready!
/hold

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierDipi

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 [pierDipi]

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

[slinkydeveloper]

/unhold

[slinkydeveloper]

I wonder how this works given the issue on the partitionkey on Slack thinking

My test uses a broker with a single partition :) I'm going to develop a test with different partitions once partitionKey is fixed

[pierDipi]

My test uses a broker with a single partition :) I'm going to develop a test with different partitions once partitionKey is fixed

Oh, gotcha!