yifan-c commented on code in PR #47:
URL: https://github.com/apache/cassandra-sidecar/pull/47#discussion_r1218690866
##########
common/src/main/java/org/apache/cassandra/sidecar/common/NodeSettings.java:
##########
@@ -57,23 +82,41 @@ public String partitioner()
return partitioner;
}
+ @JsonProperty("sidecar")
+ public Map<String, String> sidecar()
+ {
+ return sidecar;
+ }
+
+ public String sidecarVersion()
+ {
+ return sidecar.get(VERSION);
+ }
+
/**
* {@inheritDoc}
*/
- public boolean equals(Object o)
+ public boolean equals(Object other)
Review Comment:
Please do not add those unrelated changes (e.g. unnecessary variable
renaming) for future contributions.
##########
src/main/java/org/apache/cassandra/sidecar/MainModule.java:
##########
@@ -304,4 +314,28 @@ public ChecksumVerifier checksumVerifier(Vertx vertx)
{
return new MD5ChecksumVerifier(vertx.fileSystem());
}
+
+ @Provides
+ @Singleton
+ @Named("SidecarVersion")
+ private static String sidecarVersion()
Review Comment:
nit: drop `static`. I do not think it is required. The scope is singleton
already.
Does the annotation work with `private` methods? It feel counterintuitive
semantically, even if it works. Can you change it to `public`?
##########
common/src/main/java/org/apache/cassandra/sidecar/common/NodeSettings.java:
##########
@@ -57,23 +82,41 @@ public String partitioner()
return partitioner;
}
+ @JsonProperty("sidecar")
+ public Map<String, String> sidecar()
+ {
+ return sidecar;
+ }
+
+ public String sidecarVersion()
+ {
+ return sidecar.get(VERSION);
Review Comment:
can you do a null check for `sidecar`? It is prone to NPE.
##########
src/main/java/org/apache/cassandra/sidecar/MainModule.java:
##########
@@ -304,4 +314,28 @@ public ChecksumVerifier checksumVerifier(Vertx vertx)
{
return new MD5ChecksumVerifier(vertx.fileSystem());
}
+
+ @Provides
+ @Singleton
+ @Named("SidecarVersion")
+ private static String sidecarVersion()
+ {
+ final String resource = "/sidecar.version";
Review Comment:
remove `final`
##########
src/main/java/org/apache/cassandra/sidecar/MainModule.java:
##########
@@ -304,4 +314,28 @@ public ChecksumVerifier checksumVerifier(Vertx vertx)
{
return new MD5ChecksumVerifier(vertx.fileSystem());
}
+
+ @Provides
+ @Singleton
+ @Named("SidecarVersion")
+ private static String sidecarVersion()
+ {
+ final String resource = "/sidecar.version";
+ try (InputStream input =
NodeSettings.class.getResourceAsStream(resource);
+ ByteArrayOutputStream output = new ByteArrayOutputStream())
+ {
+ byte[] buffer = new byte[32];
+ int length;
+ while ((length = input.read(buffer)) >= 0)
+ {
+ output.write(buffer, 0, length);
+ }
+ return output.toString(StandardCharsets.UTF_8.name());
+ }
+ catch (Exception exception)
+ {
+ LOGGER.error("Failed to retrieve Sidecar version", exception);
+ }
+ return "unknown";
+ }
Review Comment:
There is no test for the method (reading from the resource stream). Can you
add a test?
--
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
