You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "machi1990 (via GitHub)" <gi...@apache.org> on 2023/07/06 11:16:23 UTC
[GitHub] [kafka] machi1990 opened a new pull request, #13966: Kafka 15156: Update cipherInformation correctly in DefaultChannelMetadataRegistry
machi1990 opened a new pull request, #13966:
URL: https://github.com/apache/kafka/pull/13966
1. Update cipherInformation correctly in DefaultChannelMetadataRegistry
2. Also fix a typo noticed in ChannelMetadataRegistry
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on pull request #13966: Kafka 15156: Update cipherInformation correctly in DefaultChannelMetadataRegistry
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on PR #13966:
URL: https://github.com/apache/kafka/pull/13966#issuecomment-1623493481
ping @divijvaidya can you have a look? Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 closed pull request #13966: KAFKA-15156: Update cipherInformation correctly in DefaultChannelMetadataRegistry
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 closed pull request #13966: KAFKA-15156: Update cipherInformation correctly in DefaultChannelMetadataRegistry
URL: https://github.com/apache/kafka/pull/13966
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13966: KAFKA-15156: Update cipherInformation correctly in DefaultChannelMetadataRegistry
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13966:
URL: https://github.com/apache/kafka/pull/13966#discussion_r1254327272
##########
clients/src/main/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##########
@@ -16,15 +16,15 @@
*/
package org.apache.kafka.common.network;
+import java.util.Objects;
+
public class DefaultChannelMetadataRegistry implements ChannelMetadataRegistry {
private CipherInformation cipherInformation;
private ClientInformation clientInformation;
@Override
public void registerCipherInformation(final CipherInformation cipherInformation) {
- if (this.cipherInformation != null) {
- this.cipherInformation = cipherInformation;
- }
+ this.cipherInformation = Objects.requireNonNull(cipherInformation);
Review Comment:
+1 to add a unit test.
The previous implementation will not update the cipherInformation if one already exists. This is inconsistent with the implementation in other implementation of `ChannelMetadataRegistry` i.e. SelectorChannelMetadataRegistry, hence, makes me believe that the expected behaviour is to replace the member if it already exists. Note that this file is only used in tests so far.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a diff in pull request #13966: KAFKA-15156: Update cipherInformation correctly in DefaultChannelMetadataRegistry
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13966:
URL: https://github.com/apache/kafka/pull/13966#discussion_r1254320546
##########
clients/src/main/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##########
@@ -16,15 +16,15 @@
*/
package org.apache.kafka.common.network;
+import java.util.Objects;
+
public class DefaultChannelMetadataRegistry implements ChannelMetadataRegistry {
private CipherInformation cipherInformation;
private ClientInformation clientInformation;
@Override
public void registerCipherInformation(final CipherInformation cipherInformation) {
- if (this.cipherInformation != null) {
- this.cipherInformation = cipherInformation;
- }
+ this.cipherInformation = Objects.requireNonNull(cipherInformation);
Review Comment:
Could you elaborate on what is wrong with the previous implementation? If there is a bug, would it be possible to reproduce it in a unit test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on a diff in pull request #13966: KAFKA-15156: Update cipherInformation correctly in DefaultChannelMetadataRegistry
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on code in PR #13966:
URL: https://github.com/apache/kafka/pull/13966#discussion_r1266704046
##########
clients/src/main/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##########
@@ -16,15 +16,15 @@
*/
package org.apache.kafka.common.network;
+import java.util.Objects;
+
public class DefaultChannelMetadataRegistry implements ChannelMetadataRegistry {
private CipherInformation cipherInformation;
private ClientInformation clientInformation;
@Override
public void registerCipherInformation(final CipherInformation cipherInformation) {
- if (this.cipherInformation != null) {
- this.cipherInformation = cipherInformation;
- }
+ this.cipherInformation = Objects.requireNonNull(cipherInformation);
Review Comment:
Thanks for the reply. I've updated the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on pull request #13966: KAFKA-15156: Update cipherInformation correctly in DefaultChannelMetadataRegistry
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on PR #13966:
URL: https://github.com/apache/kafka/pull/13966#issuecomment-1692115972
I've limited bandwidth thus I won't be able to carry this work forward. I'll happily close this MR and unassign myself from the JIRA for someone else to take it over.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on a diff in pull request #13966: KAFKA-15156: Update cipherInformation correctly in DefaultChannelMetadataRegistry
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on code in PR #13966:
URL: https://github.com/apache/kafka/pull/13966#discussion_r1254374926
##########
clients/src/main/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##########
@@ -16,15 +16,15 @@
*/
package org.apache.kafka.common.network;
+import java.util.Objects;
+
public class DefaultChannelMetadataRegistry implements ChannelMetadataRegistry {
private CipherInformation cipherInformation;
private ClientInformation clientInformation;
@Override
public void registerCipherInformation(final CipherInformation cipherInformation) {
- if (this.cipherInformation != null) {
- this.cipherInformation = cipherInformation;
- }
+ this.cipherInformation = Objects.requireNonNull(cipherInformation);
Review Comment:
> I am not sure about the Objects.requireNonNull though.
can you expand on this @dajac ?
> As this file is only used in tests, it would also make sense to move it to the test source tree.
I am okay with this. Will we want to keep the added unit test in this case, any thoughts? @dajac @divijvaidya
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #13966: KAFKA-15156: Update cipherInformation correctly in DefaultChannelMetadataRegistry
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #13966:
URL: https://github.com/apache/kafka/pull/13966#issuecomment-1641581680
@machi1990 please check test failures
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on pull request #13966: KAFKA-15156: Update cipherInformation correctly in DefaultChannelMetadataRegistry
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on PR #13966:
URL: https://github.com/apache/kafka/pull/13966#issuecomment-1641607411
> @machi1990 please check test failures
The CI issue was due to an issue which has been fixed by https://github.com/apache/kafka/pull/14037
I've rebased on trunk, and hopeful that should fix the issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on a diff in pull request #13966: KAFKA-15156: Update cipherInformation correctly in DefaultChannelMetadataRegistry
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on code in PR #13966:
URL: https://github.com/apache/kafka/pull/13966#discussion_r1265018212
##########
clients/src/main/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##########
@@ -16,15 +16,15 @@
*/
package org.apache.kafka.common.network;
+import java.util.Objects;
+
public class DefaultChannelMetadataRegistry implements ChannelMetadataRegistry {
private CipherInformation cipherInformation;
private ClientInformation clientInformation;
@Override
public void registerCipherInformation(final CipherInformation cipherInformation) {
- if (this.cipherInformation != null) {
- this.cipherInformation = cipherInformation;
- }
+ this.cipherInformation = Objects.requireNonNull(cipherInformation);
Review Comment:
ping @dajac @divijvaidya
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] machi1990 commented on a diff in pull request #13966: KAFKA-15156: Update cipherInformation correctly in DefaultChannelMetadataRegistry
Posted by "machi1990 (via GitHub)" <gi...@apache.org>.
machi1990 commented on code in PR #13966:
URL: https://github.com/apache/kafka/pull/13966#discussion_r1254349865
##########
clients/src/main/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##########
@@ -16,15 +16,15 @@
*/
package org.apache.kafka.common.network;
+import java.util.Objects;
+
public class DefaultChannelMetadataRegistry implements ChannelMetadataRegistry {
private CipherInformation cipherInformation;
private ClientInformation clientInformation;
@Override
public void registerCipherInformation(final CipherInformation cipherInformation) {
- if (this.cipherInformation != null) {
- this.cipherInformation = cipherInformation;
- }
+ this.cipherInformation = Objects.requireNonNull(cipherInformation);
Review Comment:
Thanks @dajac @divijvaidya for the review. I've added a new test following this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dajac commented on a diff in pull request #13966: KAFKA-15156: Update cipherInformation correctly in DefaultChannelMetadataRegistry
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13966:
URL: https://github.com/apache/kafka/pull/13966#discussion_r1254348119
##########
clients/src/main/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##########
@@ -16,15 +16,15 @@
*/
package org.apache.kafka.common.network;
+import java.util.Objects;
+
public class DefaultChannelMetadataRegistry implements ChannelMetadataRegistry {
private CipherInformation cipherInformation;
private ClientInformation clientInformation;
@Override
public void registerCipherInformation(final CipherInformation cipherInformation) {
- if (this.cipherInformation != null) {
- this.cipherInformation = cipherInformation;
- }
+ this.cipherInformation = Objects.requireNonNull(cipherInformation);
Review Comment:
I don't recall why we added this check. I think that the cipher can only be set once per connection so this may be the reason. I suppose that it is fine to remove it in this case. I am not sure about the `Objects.requireNonNull` though.
As this file is only used in tests, it would also make sense to move it to the `test` source tree.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on a diff in pull request #13966: KAFKA-15156: Update cipherInformation correctly in DefaultChannelMetadataRegistry
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on code in PR #13966:
URL: https://github.com/apache/kafka/pull/13966#discussion_r1266621190
##########
clients/src/main/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##########
@@ -16,15 +16,15 @@
*/
package org.apache.kafka.common.network;
+import java.util.Objects;
+
public class DefaultChannelMetadataRegistry implements ChannelMetadataRegistry {
private CipherInformation cipherInformation;
private ClientInformation clientInformation;
@Override
public void registerCipherInformation(final CipherInformation cipherInformation) {
- if (this.cipherInformation != null) {
- this.cipherInformation = cipherInformation;
- }
+ this.cipherInformation = Objects.requireNonNull(cipherInformation);
Review Comment:
> Will we want to keep the added unit test in this case, any thoughts?
No. unit test for a test will not be required in that case. Let's move that file to tests. Although it's a public class but it cannot be used outside of Kafka code base.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org