[
https://issues.apache.org/jira/browse/CASSANDRA-20404?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17937213#comment-17937213
]
Maulin Vasavada commented on CASSANDRA-20404:
---------------------------------------------
Thanks [~Jyothsnakonisa] .
[~smiklosovic] I added more test to validate the required changes in the
LoaderOptionsTest. Now the test will fail if we remove the code changes that
are required to use the EncryptionOptions#Builder. However, the current test
still can't validate for the server encryption options the same way since there
is no command line options in the LoaderOptions that allow Server encryption
options override. I thought of adding that support but I don't think we should
need that (I believe these command line overrides are used by the
[sstableloder|https://cassandra.apache.org/doc/4.1/cassandra/operating/bulk_loading.html])
for the loader since the loader works as a client side only. This also raises
a question - should we remove server encryption options (and updated relevant
documentation in the sstableloader) for LoaderOptions? However, that would be
out of scope for this ticket, even if we agree to do so.
> 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
> Components: Legacy/Core
> Reporter: Maulin Vasavada
> Assignee: Maulin Vasavada
> Priority: Normal
> Fix For: 5.x
>
> Time Spent: 1h 20m
> 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
