Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore context decorator #2007

Merged
merged 4 commits into from
Dec 8, 2022

Conversation

ozangunalp
Copy link
Collaborator

@ozangunalp ozangunalp commented Nov 30, 2022

Restores ContextDecorator removed in 3.22.0. Its removal caused stream transformers to dispatch messages on EventLoopContext instead of message duplicated context. See quarkusio/quarkus#23612 (comment)

The removal of ContextDecorator was an attempt to resolve the issue of message drops on Kafka multiple partitions setup. After investigation, the message drops resulted from Mutiny https://github.com/smallrye/smallrye-mutiny/blob/main/implementation/src/main/java/io/smallrye/mutiny/helpers/HalfSerializer.java being called onItem concurrently.
In order to resolve this, this change removes unnecessary wrappings of Multi`s, which itself wraps subscribers in https://github.com/smallrye/smallrye-mutiny/blob/main/implementation/src/main/java/io/smallrye/mutiny/helpers/StrictMultiSubscriber.java

This change also moves KafkaConnector to the InboundConnector OutboundConnector API which exposes Publisher/Subscribers in Connectors instead of PublisherBuilder and SubscriberBuilder.

@ozangunalp ozangunalp force-pushed the restore_context_decorator branch from 0d704ad to b2fee86 Compare November 30, 2022 09:58
@ozangunalp ozangunalp force-pushed the restore_context_decorator branch 2 times, most recently from f3e0fa4 to 0a875ba Compare December 6, 2022 22:00
@codecov-commenter
Copy link

codecov-commenter commented Dec 6, 2022

Codecov Report

Merging #2007 (0a875ba) into main (853cb81) will increase coverage by 0.13%.
The diff coverage is 71.87%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #2007      +/-   ##
============================================
+ Coverage     77.26%   77.40%   +0.13%     
- Complexity     3462     3473      +11     
============================================
  Files           293      294       +1     
  Lines         11638    11656      +18     
  Branches       1486     1488       +2     
============================================
+ Hits           8992     9022      +30     
+ Misses         1948     1942       -6     
+ Partials        698      692       -6     
Impacted Files Coverage Δ
...o/smallrye/reactive/messaging/mqtt/MqttSource.java 84.78% <ø> (-0.33%) ⬇️
...active/messaging/providers/helpers/MultiUtils.java 45.45% <26.08%> (-44.55%) ⬇️
...ye/reactive/messaging/providers/wiring/Wiring.java 92.96% <80.00%> (ø)
...mallrye/reactive/messaging/amqp/AmqpConnector.java 83.05% <100.00%> (ø)
...llrye/reactive/messaging/kafka/KafkaConnector.java 89.47% <100.00%> (ø)
...llrye/reactive/messaging/kafka/impl/KafkaSink.java 94.03% <100.00%> (+1.18%) ⬆️
...rye/reactive/messaging/kafka/impl/KafkaSource.java 85.20% <100.00%> (-0.14%) ⬇️
...eactive/messaging/providers/ProcessorMediator.java 91.45% <100.00%> (ø)
...eactive/messaging/providers/PublisherMediator.java 78.57% <100.00%> (ø)
...messaging/providers/StreamTransformerMediator.java 94.20% <100.00%> (+0.17%) ⬆️
... and 20 more

@ozangunalp ozangunalp marked this pull request as ready for review December 7, 2022 09:04
@ozangunalp ozangunalp added this to the 3.22.1 milestone Dec 7, 2022
@ozangunalp ozangunalp force-pushed the restore_context_decorator branch from 0a875ba to 9e1a587 Compare December 7, 2022 16:31
cescoffier
cescoffier previously approved these changes Dec 8, 2022
@@ -110,7 +110,7 @@
<jandex-maven-plugin.version>1.2.3</jandex-maven-plugin.version>

<nats-embedded.version>2.1.0</nats-embedded.version>
<slf4j-log4j12.version>2.0.5</slf4j-log4j12.version>
<slf4j-log4j12.version>1.7.36</slf4j-log4j12.version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should ban slf4j 2 for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fixed the dependabot config (I think)

@cescoffier cescoffier merged commit dbddc32 into smallrye:main Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants