KAFKA-15156: Update cipherInformation correctly in DefaultChannelMetadataRegistry

[machi1990]

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)

[machi1990]

ping @divijvaidya can you have a look? Thanks

[dajac]

Thanks for the PR. I left a comment below.

[dajac]

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?

[divijvaidya]

+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.

[dajac]

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.

[machi1990]

Thanks @dajac @divijvaidya for the review. I've added a new test following this change.

[machi1990]

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

[machi1990]

ping @dajac @divijvaidya

[divijvaidya]

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.

[machi1990]

Thanks for the reply. I've updated the PR.

[divijvaidya]

@machi1990 please check test failures

[machi1990]

@machi1990 please check test failures

The CI issue was due to an issue which has been fixed by #14037
I've rebased on trunk, and hopeful that should fix the issue.

[machi1990]

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.