[
https://issues.apache.org/jira/browse/CASSANDRA-20404?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17935670#comment-17935670
]
Maulin Vasavada edited comment on CASSANDRA-20404 at 3/14/25 11:52 PM:
-----------------------------------------------------------------------
Hi [~smiklosovic] I raised a
[PR-3985|https://github.com/apache/cassandra/pull/3985]. Most of the changes
are in Tests for using builders now. Additional changes are in 'tools' that
used the 'with' methods from the Encryption Options. I see one test
(HandshakeTest#
testOutboundConnectionfFallbackDuringUpgrades()) failing out of testing all the
changed tests + all SSL related tests. I don't think its related to my changes
but will look into it now. Wanted to give more time to you guys to see my
changes.
One key verification of the changes would be - What it takes now to add a new
field in the EncryptionOptions config. If that only requires adding that field
specific changes within constructor assignments and adding specific build
method for it, I think this refactoring served its purpose. Hence please
evaluate the changes with that lence.
Thanks
was (Author: maulin.vasavada):
Hi [~smiklosovic] I raised a
[PR-3985|https://github.com/apache/cassandra/pull/3985]. Most of the changes
are in Tests for using builders now. Additional changes are in 'tools' that
used the 'with' methods from the Encryption Options. I see two tests failing
out of testing all the changed tests + all SSL related tests. I don't think its
related to my changes but will look into it now. Wanted to give more time to
you guys to see my changes.
One key verification of the changes would be - What it takes now to add a new
field in the EncryptionOptions config. If that only requires adding that field
specific changes within constructor assignments and adding specific build
method for it, I think this refactoring served its purpose. Hence please
evaluate the changes with that lence.
Thanks
> Improve EncryptionOptions class's `with` methods using Builder pattern
> ----------------------------------------------------------------------
>
> Key: CASSANDRA-20404
> URL: https://issues.apache.org/jira/browse/CASSANDRA-20404
> Project: Apache Cassandra
> Issue Type: Improvement
> Reporter: Maulin Vasavada
> Assignee: Maulin Vasavada
> Priority: Normal
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Currently,
> [EncryptionOptions.java|https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/config/EncryptionOptions.java#L520]
> allows constructing the object using `with` prefix method pattern which
> sounds similar to the Builder Pattern
> ([example)|https://blogs.oracle.com/javamagazine/post/exploring-joshua-blochs-builder-design-pattern-in-java]
> However, when there is any new parameter introduced in the class for a
> corresponding configuration option, it requires changes to all the
> constructor calls from those `with` methods to reflect the new parameter.
> This is error prone given so many string parameters and considering some
> [special
> use-cases|https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/config/EncryptionOptions.java#L1190]
> it makes it challenging to modify the code for a simple task - adding a new
> parameter in the class.
> Proposal here is to use the Builder pattern (like shows in the example linked
> above) to improve the code in EncryptionOptions.java file.
> There could be a potential to consider using "java record" type that got
> introduced in Java 14+ or using [Lombok plugin|https://projectlombok.org/]
> but in order to be backward compatible and avoid additional dependency on
> external packages- may be we can simply follow the builder pattern.
> One change that we have to be careful about is - the new builder pattern
> implementation may not call `applyConfig()` everytime upon calling `with`
> methods compared to the current implementation. Only when final
> `Builder#build()` happens it will call the applyConfig() method . While that
> saves multiple initialization that happen today in Tests due to heavy usage,
> we have to assess if there is any impact due to this behavior change.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
