bianca-stanciu29 commented on code in PR #223:
URL:
https://github.com/apache/cassandra-analytics/pull/223#discussion_r4060570465
##########
cassandra-analytics-core/src/main/java/org/apache/cassandra/spark/bulkwriter/CassandraClusterInfo.java:
##########
@@ -138,6 +138,57 @@ public CassandraClusterInfo(BroadcastableClusterInfo
broadcastable)
this.allNodeSettingFutures = null;
}
+ /**
+ * Creates a {@link CassandraClusterInfo} for a single cluster, selecting
the concrete type based on the
+ * {@link BulkSparkConf#sidecarBehindLoadBalancer} flag.
+ *
+ * @param conf bulk write conf
+ * @return {@link LoadBalancedCassandraClusterInfo} when Sidecar is behind
a load balancer, otherwise a plain
+ * {@link CassandraClusterInfo}
+ */
+ public static CassandraClusterInfo create(BulkSparkConf conf)
Review Comment:
Good point, thanks.
I agree that having the base class reference its subclass in `create()`
isn’t ideal.
I think the abstract CassandraClusterInfo + StandaloneCassandraClusterInfo /
LoadBalancedCassandraClusterInfo approach makes sense. My only hesitation is
the scope. CassandraClusterInfo is quite central, and making it abstract would
mean introducing a separate StandaloneCassandraClusterInfo and updating the
places that currently instantiate or mock CassandraClusterInfo directly. That
would likely touch a number of tests and mocks as well.
Since the PR is otherwise ready, I’d prefer to keep this change smaller for
now and revisit the full split separately. But happy to go with the cleaner
approach if you think it’s worth doing as part of this 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
