-
Notifications
You must be signed in to change notification settings - Fork 1.6k
GH-3989 Consider the custom name of the reply topic name in sendAndRec… #3994
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
Conversation
99a759d
to
95c9166
Compare
@mipo256 PR build is failing, just letting you know. |
Hey @sobychacko ! I know, thanks! I'll fix the checkstyle violations shortly and then ping you :) |
Hey guys @sobychacko, @artembilan. I've added some test cases for this scenario and introduced minor polishing, the PR is ready for review |
...g-kafka/src/test/java/org/springframework/kafka/requestreply/ReplyingKafkaTemplateTests.java
Outdated
Show resolved
Hide resolved
...g-kafka/src/test/java/org/springframework/kafka/requestreply/ReplyingKafkaTemplateTests.java
Outdated
Show resolved
Hide resolved
...g-kafka/src/test/java/org/springframework/kafka/requestreply/ReplyingKafkaTemplateTests.java
Outdated
Show resolved
Hide resolved
...g-kafka/src/test/java/org/springframework/kafka/requestreply/ReplyingKafkaTemplateTests.java
Outdated
Show resolved
Hide resolved
...g-kafka/src/test/java/org/springframework/kafka/requestreply/ReplyingKafkaTemplateTests.java
Outdated
Show resolved
Hide resolved
...g-kafka/src/test/java/org/springframework/kafka/requestreply/ReplyingKafkaTemplateTests.java
Outdated
Show resolved
Hide resolved
...g-kafka/src/test/java/org/springframework/kafka/requestreply/ReplyingKafkaTemplateTests.java
Outdated
Show resolved
Hide resolved
@@ -35,8 +35,16 @@ public final class CorrelationKey { | |||
|
|||
private final byte[] correlationId; | |||
|
|||
/** | |||
* Cached hex representation of the {@link #correlationId}. | |||
* TODO: Migrate to stable values JEP 502 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate what you mean by migrate to JEP 502 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean that in JEP 502 we would have stable values in Java, which are lazily initilized final
fields. The lazily computed hashcode and hex representation of correlation id is exactly that use case.
Hey @sobychacko, can you please re-review? I've answered you questions and fixed the comments that I think we both agree upon. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have the title of PR and commit message fixed for the typo?
That Consder
word is wrong.
Thanks
Hey @artembilan, sure, it's done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know issue in details to be sure why we didn't have this feature for years, but it still worth to look into sending-messages.adoc
to determine the reason.
And mention this new feature over there if that is what we would want.
Thanks
spring-kafka/src/main/java/org/springframework/kafka/listener/DeliveryAttemptAware.java
Outdated
Show resolved
Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/requestreply/RequestReplyFuture.java
Outdated
Show resolved
Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/support/AbstractKafkaHeaderMapper.java
Outdated
Show resolved
Hide resolved
...g-kafka/src/test/java/org/springframework/kafka/requestreply/ReplyingKafkaTemplateTests.java
Outdated
Show resolved
Hide resolved
...g-kafka/src/test/java/org/springframework/kafka/requestreply/ReplyingKafkaTemplateTests.java
Outdated
Show resolved
Hide resolved
OK. I see @sobychacko said in the issue:
So, feels like this fix is supposed to be a back-port candidate. |
spring-kafka/src/main/java/org/springframework/kafka/requestreply/ReplyingKafkaTemplate.java
Outdated
Show resolved
Hide resolved
spring-kafka/src/main/java/org/springframework/kafka/requestreply/ReplyingKafkaTemplate.java
Outdated
Show resolved
Hide resolved
...g-kafka/src/test/java/org/springframework/kafka/requestreply/ReplyingKafkaTemplateTests.java
Outdated
Show resolved
Hide resolved
With all due respect Artem @artembilan, I do not think you're fully in the context of the problem.
No, it is not. And it is the core of the issue. The custom reply header is considered already, and it works fine. The problem arises when the the custom reply header is overridden by the user via public API, and in this case the custom reply header is sent in the kafka message two times. It's get duplicated, see from the original issue:
I'm reading your code review and I think we have a misunderstanding here. I'll fix all your comments regarding the code style etc, no problem, I'm just making sure we're on the same page. |
My 2 cents. I'm not sure it is a good backport candidate. This is the bug, and when it get's fixed, then hypothetically, when previously producer produced 4 headers in the message (two for custom reply header, and two for custom reply partition), now it will produce 4. For me at least, it is highly questionable if this change is backward compatible, since the consumers now will receive de-facto different messages (4 vs 2 headers). CC : @sobychacko @artembilan |
738a6ca
to
6290a30
Compare
…me in sendAndReceive Signed-off-by: mipo256 <[email protected]>
Merged via 97f482a. |
Closes #3989
WIP, writing tests