-
Notifications
You must be signed in to change notification settings - Fork 86
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
[all] Changed Kafka from LI's 2.4.1.78 to Apache's 2.4.1 #1000
Conversation
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 think PubSubSecurityProtocol
is missing from the commit
3b85901
to
7cf734f
Compare
Duh 😅 ... I pushed it up now... let's see if it works 💥 🤞 |
.../main/java/com/linkedin/venice/pubsub/adapter/kafka/producer/ApacheKafkaProducerAdapter.java
Outdated
Show resolved
Hide resolved
internal/venice-common/src/main/java/com/linkedin/venice/pubsub/api/PubSubSecurityProtocol.java
Outdated
Show resolved
Hide resolved
internal/venice-common/src/main/java/com/linkedin/venice/pubsub/api/PubSubSecurityProtocol.java
Show resolved
Hide resolved
Closing temporarily. Will put up a separate PR to refactor the VeniceWriter's close routine, so that it stops relying on the flush with timeout API. Then I'll rebase the current PR on top of that other change. |
9fa33d8
to
4c01770
Compare
internal/venice-common/src/main/java/com/linkedin/venice/pubsub/api/PubSubSecurityProtocol.java
Outdated
Show resolved
Hide resolved
Re-opened this PR, and rebased it on top of #1014, to solve the usage of the flush with timeout API, which does not exist in Apache Kafka. This should be good to go, I think... |
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.
LGTM!
Hey @FelixGV , do you have time to rebase from main and get your PR merged? 🚀 |
Sorry, I missed your comment. The reason this PR is on hold is that we need to make some changes to our proprietary wrappers (MPs) in order to be able to pull in this change safely... @sushantmane got started and found some issues, and I'm planning to continue that work, but I haven't had a chance yet. Hoping to get to it soon. But in the meantime, it means this PR is blocked... |
610713b
to
c42246c
Compare
I will close this one temporarily to jump on something else, but I do plan to get back to it soon-ish. |
Several code changes to make it work. Introduced a new PubSubSecurityProtocol to replace all usage of Kafka's SecurityProtocol enum, since that one has a different package name between the Apache and LinkedIn forks of Kafka. AK uses: org.apache.kafka.common.security.auth.SecurityProtocol While LI uses: org.apache.kafka.common.protocol.SecurityProtocol
c42246c
to
bfaf0a9
Compare
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.
LGTM! Thanks, @FelixGV!
Awesome, thanks @sushantmane ! |
…)" This reverts commit af00080.
This is a second attempt at the change from PR linkedin#1000. Several code changes to make it work. Introduced a new PubSubSecurityProtocol to replace all usage of Kafka's SecurityProtocol enum, since that one has a different package name between the Apache and LinkedIn forks of Kafka. AK uses: org.apache.kafka.common.security.auth.SecurityProtocol While LI uses: org.apache.kafka.common.protocol.SecurityProtocol
This is a second attempt at the change from PR #1000. Several code changes to make it work. Introduced a new PubSubSecurityProtocol to replace all usage of Kafka's SecurityProtocol enum, since that one has a different package name between the Apache and LinkedIn forks of Kafka. AK uses: org.apache.kafka.common.security.auth.SecurityProtocol While LI uses: org.apache.kafka.common.protocol.SecurityProtocol
Several code changes to make it work.
Introduced a new PubSubSecurityProtocol to replace all usage of Kafka's SecurityProtocol enum, since that one has a different package name between the Apache and LinkedIn forks of Kafka.
AK uses: org.apache.kafka.common.security.auth.SecurityProtocol
While LI uses: org.apache.kafka.common.protocol.SecurityProtocol
TODO: Decide what to do about the producer flush API not having support for passing a time out in Apache Kafka.
This is a step in the direction of solving #998.
How was this PR tested?
GHCI... (needs more testing though...)
Does this PR introduce any user-facing changes?