yifan-c commented on code in PR #193:
URL: https://github.com/apache/cassandra-sidecar/pull/193#discussion_r1974514376
##########
server/src/main/java/org/apache/cassandra/sidecar/db/ConfigAccessorFactory.java:
##########
@@ -25,29 +27,22 @@
*/
public class ConfigAccessorFactory
{
- private final KafkaConfigAccessor kafkaConfigAccessor;
- private final CdcConfigAccessor cdcConfigAccessor;
+ private final Map<Service, ConfigAccessor> configAccessors = new
HashMap<>();
@Inject
public ConfigAccessorFactory(KafkaConfigAccessor kafkaConfigAccessor,
CdcConfigAccessor cdcConfigAccessor)
{
- this.kafkaConfigAccessor = kafkaConfigAccessor;
- this.cdcConfigAccessor = cdcConfigAccessor;
+ configAccessors.put(Service.KAFKA, kafkaConfigAccessor);
+ configAccessors.put(Service.CDC, cdcConfigAccessor);
}
- public ConfigAccessor getConfigAccessor(final String service)
+ public ConfigAccessor getConfigAccessor(final Service service)
{
- if (service.equals(Service.KAFKA.serviceName))
+ if (configAccessors.containsKey(service))
{
- return kafkaConfigAccessor;
+ return configAccessors.get(service);
}
-
- if (service.equals(Service.CDC.serviceName))
- {
- return cdcConfigAccessor;
- }
-
throw new RuntimeException("Couldn't find a db accessor for service "
+ service);
}
Review Comment:
prefer not containing `get` in the method name, remove `final` and look up
just once.
```suggestion
public ConfigAccessor configAccessor(Service service)
{
ConfigAccessor accessor = configAccessors.get(service);
if (accessor == null)
{
throw new RuntimeException("Couldn't find a db accessor for
service " + service);
}
return accessor;
}
```
##########
server/src/main/java/org/apache/cassandra/sidecar/db/ConfigAccessorFactory.java:
##########
@@ -17,6 +17,8 @@
*/
package org.apache.cassandra.sidecar.db;
+import java.util.HashMap;
Review Comment:
remove this import.
##########
server/src/main/java/org/apache/cassandra/sidecar/cdc/CdcConfigImpl.java:
##########
@@ -333,4 +334,29 @@ public ScheduleDecision scheduleDecision()
return shouldSkip ? ScheduleDecision.SKIP :
ScheduleDecision.EXECUTE;
}
}
+
+ enum ConfigKeys
+ {
+ DC("dc"),
+ LOG_ONLY("log_only"),
+ PERSIST_STATE("persist_state"),
+ ENV("env"),
+ KAFKA_TOPIC("topic"),
+ KAFKA_TOPIC_FORMAT_TYPE("topic_format_type"),
+ CDC_ENABLED("cdc_enabled"),
+ JOB_ID("jobId"),
+ WATERMARK_SECONDS("watermark_seconds"),
+ MICRO_BATCH_DELAY_IN_MILLIS("microbatch_delay_millis"),
+ MAX_COMMIT_LOGS("max_commit_logs"),
+ MAX_WATERMARKER_SIZE("max_watermarker_size"),
+ FAIL_KAFKA_ERRORS("fail_kafka_errors"),
+ FAIL_KAFKA_TOO_LARGE_ERRORS("fail_kafka_too_large_errors"),
+ PERSIST_DELAY_MILLIS("persist_delay_millis");
+ private final String name;
+
+ ConfigKeys(String name)
+ {
+ this.name = name;
+ }
+ }
Review Comment:
There is no need to repeat the string..
```suggestion
enum ConfigKeys
{
DC,
LOG_ONLY,
PERSIST_STATE,
ENV,
KAFKA_TOPIC,
KAFKA_TOPIC_FORMAT_TYPE,
CDC_ENABLED,
JOB_ID,
WATERMARK_SECONDS,
MICRO_BATCH_DELAY_IN_MILLIS,
MAX_COMMIT_LOGS,
MAX_WATERMARKER_SIZE,
FAIL_KAFKA_ERRORS,
FAIL_KAFKA_TOO_LARGE_ERRORS,
PERSIST_DELAY_MILLIS;
private final String lowcaseName;
ConfigKeys()
{
this.lowcaseName = name().toLowerCase(Locale.ENGLISH);
}
}
```
##########
server/src/main/java/org/apache/cassandra/sidecar/db/ConfigAccessorFactory.java:
##########
@@ -25,29 +27,22 @@
*/
public class ConfigAccessorFactory
{
- private final KafkaConfigAccessor kafkaConfigAccessor;
- private final CdcConfigAccessor cdcConfigAccessor;
+ private final Map<Service, ConfigAccessor> configAccessors = new
HashMap<>();
Review Comment:
```suggestion
private final Map<Service, ConfigAccessor> configAccessors;
```
##########
server/src/main/java/org/apache/cassandra/sidecar/routes/cdc/Service.java:
##########
@@ -26,9 +29,27 @@ public enum Service
CDC("cdc");
public final String serviceName;
+ private static final Map<String, Service> LOOKUP = new HashMap<>();
+
+ static
+ {
+ for (Service service : Service.values())
+ {
+ LOOKUP.put(service.serviceName, service);
+ }
+ }
Review Comment:
remove this code. It is unnecessary.
##########
server/src/main/java/org/apache/cassandra/sidecar/routes/cdc/Service.java:
##########
@@ -26,9 +29,27 @@ public enum Service
CDC("cdc");
public final String serviceName;
+ private static final Map<String, Service> LOOKUP = new HashMap<>();
+
+ static
+ {
+ for (Service service : Service.values())
+ {
+ LOOKUP.put(service.serviceName, service);
+ }
+ }
Service(final String serviceName)
{
this.serviceName = serviceName;
}
+
+ public static Service withName(String serviceName)
+ {
+ if (LOOKUP.containsKey(serviceName))
+ {
+ return LOOKUP.get(serviceName);
+ }
+ throw new RuntimeException("Invalid service name " + serviceName);
+ }
Review Comment:
This method is unnecessary. You can remove it. But if you want to provide
detailed error context. Here is my suggestion
```suggestion
public static Service withName(String serviceName)
{
try
{
return Service.valueOf(serviceName);
}
catch (Exception cause)
{
throw new RuntimeException("Invalid service name " +
serviceName, cause);
}
}
```
##########
server/src/main/java/org/apache/cassandra/sidecar/routes/cdc/Service.java:
##########
@@ -26,9 +29,27 @@ public enum Service
CDC("cdc");
Review Comment:
There is no need to define the string value that is identical to the enum
##########
server/src/main/java/org/apache/cassandra/sidecar/routes/cdc/ServiceConfigValidator.java:
##########
@@ -35,9 +35,13 @@
@Singleton
public class ServiceConfigValidator
{
- public void validateService(String requestService)
+ public Service validateAndGet(String requestService)
{
- if (Stream.of(Service.values()).noneMatch(v ->
v.serviceName.equals(requestService)))
+ try
+ {
+ return Service.withName(requestService);
Review Comment:
TBH, remove `withName` and just call `Service.valueOf(...)` here.
##########
server/src/main/java/org/apache/cassandra/sidecar/db/ConfigAccessorFactory.java:
##########
@@ -25,29 +27,22 @@
*/
public class ConfigAccessorFactory
{
- private final KafkaConfigAccessor kafkaConfigAccessor;
- private final CdcConfigAccessor cdcConfigAccessor;
+ private final Map<Service, ConfigAccessor> configAccessors = new
HashMap<>();
@Inject
public ConfigAccessorFactory(KafkaConfigAccessor kafkaConfigAccessor,
CdcConfigAccessor cdcConfigAccessor)
{
- this.kafkaConfigAccessor = kafkaConfigAccessor;
- this.cdcConfigAccessor = cdcConfigAccessor;
+ configAccessors.put(Service.KAFKA, kafkaConfigAccessor);
+ configAccessors.put(Service.CDC, cdcConfigAccessor);
Review Comment:
create immutable map.
```suggestion
configAccessors = Map.of(Service.KAFKA, kafkaConfigAccessor,
Service.CDC, cdcConfigAccessor);
```
--
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: pr-unsubscr...@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org
For additional commands, e-mail: pr-h...@cassandra.apache.org
