Re: [PR] KAFKA-15156 Update cipherInformation correctly in DefaultChannelMetadataRegistry [kafka]
chia7712 commented on PR #16169: URL: https://github.com/apache/kafka/pull/16169#issuecomment-2146219949 > Did we mean to totally remove the null check here? Pardon me, the topic is related to "update" of cipherInformation, and this PR has fixed that. If you prefer to either keep checks or have more checks, I can file a PR to add consistent/explicit checks to both testing and production code. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15156 Update cipherInformation correctly in DefaultChannelMetadataRegistry [kafka]
jolshan commented on PR #16169: URL: https://github.com/apache/kafka/pull/16169#issuecomment-2146083639 Did we mean to totally remove the null check here? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15156 Update cipherInformation correctly in DefaultChannelMetadataRegistry [kafka]
chia7712 merged PR #16169: URL: https://github.com/apache/kafka/pull/16169 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15156 Update cipherInformation correctly in DefaultChannelMetadataRegistry [kafka]
ganesh-sadanala commented on code in PR #16169:
URL: https://github.com/apache/kafka/pull/16169#discussion_r1623645445
##
clients/src/test/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##
@@ -22,9 +22,10 @@ public class DefaultChannelMetadataRegistry implements
ChannelMetadataRegistry {
@Override
public void registerCipherInformation(final CipherInformation
cipherInformation) {
-if (this.cipherInformation != null) {
-this.cipherInformation = cipherInformation;
+if (cipherInformation == null) {
+throw new IllegalArgumentException("cipherInformation cannot be
null");
Review Comment:
Done. Please review it
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15156 Update cipherInformation correctly in DefaultChannelMetadataRegistry [kafka]
chia7712 commented on code in PR #16169:
URL: https://github.com/apache/kafka/pull/16169#discussion_r1623644649
##
clients/src/test/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##
@@ -22,9 +22,10 @@ public class DefaultChannelMetadataRegistry implements
ChannelMetadataRegistry {
@Override
public void registerCipherInformation(final CipherInformation
cipherInformation) {
-if (this.cipherInformation != null) {
-this.cipherInformation = cipherInformation;
+if (cipherInformation == null) {
+throw new IllegalArgumentException("cipherInformation cannot be
null");
Review Comment:
> Shall i remove the exception?
yep, I prefer this way
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15156 Update cipherInformation correctly in DefaultChannelMetadataRegistry [kafka]
ganesh-sadanala commented on code in PR #16169:
URL: https://github.com/apache/kafka/pull/16169#discussion_r1623643756
##
clients/src/test/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##
@@ -22,9 +22,10 @@ public class DefaultChannelMetadataRegistry implements
ChannelMetadataRegistry {
@Override
public void registerCipherInformation(final CipherInformation
cipherInformation) {
-if (this.cipherInformation != null) {
-this.cipherInformation = cipherInformation;
+if (cipherInformation == null) {
+throw new IllegalArgumentException("cipherInformation cannot be
null");
Review Comment:
Shall i remove the exception? or shall I update the code to this for now?
```
if(cipherInformation != null) this.cipherInformation=cipherInformation;
```
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15156 Update cipherInformation correctly in DefaultChannelMetadataRegistry [kafka]
ganesh-sadanala commented on code in PR #16169:
URL: https://github.com/apache/kafka/pull/16169#discussion_r1623643756
##
clients/src/test/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##
@@ -22,9 +22,10 @@ public class DefaultChannelMetadataRegistry implements
ChannelMetadataRegistry {
@Override
public void registerCipherInformation(final CipherInformation
cipherInformation) {
-if (this.cipherInformation != null) {
-this.cipherInformation = cipherInformation;
+if (cipherInformation == null) {
+throw new IllegalArgumentException("cipherInformation cannot be
null");
Review Comment:
Shall i remove the exception?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15156 Update cipherInformation correctly in DefaultChannelMetadataRegistry [kafka]
chia7712 commented on code in PR #16169:
URL: https://github.com/apache/kafka/pull/16169#discussion_r1623643481
##
clients/src/test/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##
@@ -22,9 +22,10 @@ public class DefaultChannelMetadataRegistry implements
ChannelMetadataRegistry {
@Override
public void registerCipherInformation(final CipherInformation
cipherInformation) {
-if (this.cipherInformation != null) {
-this.cipherInformation = cipherInformation;
+if (cipherInformation == null) {
+throw new IllegalArgumentException("cipherInformation cannot be
null");
Review Comment:
agree to consistence. We can throw exception in the future if extra checks
are added into production code
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15156 Update cipherInformation correctly in DefaultChannelMetadataRegistry [kafka]
ganesh-sadanala commented on code in PR #16169:
URL: https://github.com/apache/kafka/pull/16169#discussion_r1623639224
##
clients/src/test/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##
@@ -22,9 +22,10 @@ public class DefaultChannelMetadataRegistry implements
ChannelMetadataRegistry {
@Override
public void registerCipherInformation(final CipherInformation
cipherInformation) {
-if (this.cipherInformation != null) {
-this.cipherInformation = cipherInformation;
+if (cipherInformation == null) {
+throw new IllegalArgumentException("cipherInformation cannot be
null");
Review Comment:
Since there are no invocation of the above method, no tests have failed in
local
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15156 Update cipherInformation correctly in DefaultChannelMetadataRegistry [kafka]
ganesh-sadanala commented on code in PR #16169:
URL: https://github.com/apache/kafka/pull/16169#discussion_r1623637352
##
clients/src/test/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##
@@ -22,9 +22,10 @@ public class DefaultChannelMetadataRegistry implements
ChannelMetadataRegistry {
@Override
public void registerCipherInformation(final CipherInformation
cipherInformation) {
-if (this.cipherInformation != null) {
-this.cipherInformation = cipherInformation;
+if (cipherInformation == null) {
+throw new IllegalArgumentException("cipherInformation cannot be
null");
Review Comment:
I see that `SelectorChannelMetadataRegistry#registerCipherInformation` does
not throw any such exception. The proposed change for
`DefaultChannelMetadataRegistry` is because it is only used in test classes,
not for production. However, I see that there are no invocations of
`DefaultChannelMetadataRegistry#registerCipherInformation` so far. In my
opinion to keep it consistent, either both the classes should have exception
thrown and handled properly, otherwise since
`SelectorChannelMetadataRegistry#registerCipherInformation` is currently used
in production and it does not have the exception thrown,
`DefaultChannelMetadataRegistry#registerCipherInformation` does not require any
exception. @divijvaidya @dajac You want to add anything? I guess the author
suggested to throw the exception for future use.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15156 Update cipherInformation correctly in DefaultChannelMetadataRegistry [kafka]
ganesh-sadanala commented on code in PR #16169:
URL: https://github.com/apache/kafka/pull/16169#discussion_r1623637352
##
clients/src/test/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##
@@ -22,9 +22,10 @@ public class DefaultChannelMetadataRegistry implements
ChannelMetadataRegistry {
@Override
public void registerCipherInformation(final CipherInformation
cipherInformation) {
-if (this.cipherInformation != null) {
-this.cipherInformation = cipherInformation;
+if (cipherInformation == null) {
+throw new IllegalArgumentException("cipherInformation cannot be
null");
Review Comment:
I see that `SelectorChannelMetadataRegistry#registerCipherInformation` does
not throw any such exception. The proposed change for
`DefaultChannelMetadataRegistry` is because it is only used in test classes,
not for production. However, I see that there are no invocations of
`DefaultChannelMetadataRegistry#registerCipherInformation` so far. In my
opinion to keep it consistent, either both the classes should have exception
thrown and handled properly, otherwise since
`SelectorChannelMetadataRegistry#registerCipherInformation` is currently used
in production and it does not have the exception thrown,
`DefaultChannelMetadataRegistry#registerCipherInformation` does not require any
exception needed. @divijvaidya @dajac You want to add anything? I guess the
author suggested to throw the exception for future use.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15156 Update cipherInformation correctly in DefaultChannelMetadataRegistry [kafka]
ganesh-sadanala commented on code in PR #16169:
URL: https://github.com/apache/kafka/pull/16169#discussion_r1623637352
##
clients/src/test/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##
@@ -22,9 +22,10 @@ public class DefaultChannelMetadataRegistry implements
ChannelMetadataRegistry {
@Override
public void registerCipherInformation(final CipherInformation
cipherInformation) {
-if (this.cipherInformation != null) {
-this.cipherInformation = cipherInformation;
+if (cipherInformation == null) {
+throw new IllegalArgumentException("cipherInformation cannot be
null");
Review Comment:
I see that `SelectorChannelMetadataRegistry#registerCipherInformation` does
not throw any such exception. The proposed change for
`DefaultChannelMetadataRegistry` is because it is only used in test classes,
not for production. @divijvaidya @dajac You want to add anything?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15156 Update cipherInformation correctly in DefaultChannelMetadataRegistry [kafka]
ganesh-sadanala commented on code in PR #16169:
URL: https://github.com/apache/kafka/pull/16169#discussion_r1623637352
##
clients/src/test/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##
@@ -22,9 +22,10 @@ public class DefaultChannelMetadataRegistry implements
ChannelMetadataRegistry {
@Override
public void registerCipherInformation(final CipherInformation
cipherInformation) {
-if (this.cipherInformation != null) {
-this.cipherInformation = cipherInformation;
+if (cipherInformation == null) {
+throw new IllegalArgumentException("cipherInformation cannot be
null");
Review Comment:
I see that `SelectorChannelMetadataRegistry#registerCipherInformation` does
not throw any such exception. The proposed change for
`DefaultChannelMetadataRegistry` is because it is only test classes, not for
production. @divijvaidya @dajac You want to add anything?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15156 Update cipherInformation correctly in DefaultChannelMetadataRegistry [kafka]
ganesh-sadanala commented on code in PR #16169:
URL: https://github.com/apache/kafka/pull/16169#discussion_r1623637352
##
clients/src/test/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##
@@ -22,9 +22,10 @@ public class DefaultChannelMetadataRegistry implements
ChannelMetadataRegistry {
@Override
public void registerCipherInformation(final CipherInformation
cipherInformation) {
-if (this.cipherInformation != null) {
-this.cipherInformation = cipherInformation;
+if (cipherInformation == null) {
+throw new IllegalArgumentException("cipherInformation cannot be
null");
Review Comment:
I see that `SelectorChannelMetadataRegistry#registerCipherInformation` does
not throw any such exception. The proposed change for
`DefaultChannelMetadataRegistry` is because it is only used in running test
cases, not for production. @divijvaidya @dajac You want to add anything?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15156 Update cipherInformation correctly in DefaultChannelMetadataRegistry [kafka]
chia7712 commented on code in PR #16169:
URL: https://github.com/apache/kafka/pull/16169#discussion_r1623634685
##
clients/src/test/java/org/apache/kafka/common/network/DefaultChannelMetadataRegistry.java:
##
@@ -22,9 +22,10 @@ public class DefaultChannelMetadataRegistry implements
ChannelMetadataRegistry {
@Override
public void registerCipherInformation(final CipherInformation
cipherInformation) {
-if (this.cipherInformation != null) {
-this.cipherInformation = cipherInformation;
+if (cipherInformation == null) {
+throw new IllegalArgumentException("cipherInformation cannot be
null");
Review Comment:
It would be nice that the thrown exception is same to
`SelectorChannelMetadataRegistry#registerCipherInformation`. Could you check
that?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[PR] KAFKA-15156 Update cipherInformation correctly in DefaultChannelMetadataRegistry [kafka]
ganesh-sadanala opened a new pull request, #16169: URL: https://github.com/apache/kafka/pull/16169 Assing the new value of `cipherInformation` to a member variable if the assigning variable value is not null. The previous implementation has a bug preventing it from getting updated with the new value. *Summary of testing strategy (including rationale) for the feature or bug fix. Unit and/or integration tests are expected for any behaviour change and system tests should be considered for larger changes.* ### Committer Checklist (excluded from commit message) - [x] Verify design and implementation - [x] Verify test coverage and CI build status - [x] Verify documentation (including upgrade notes) @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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
