abhishekrb19 commented on code in PR #19123:
URL: https://github.com/apache/druid/pull/19123#discussion_r2978512422
##########
server/src/main/java/org/apache/druid/server/emitter/EmitterModule.java:
##########
@@ -177,4 +184,26 @@ public Emitter get()
return emitter;
}
}
+
+ /**
+ * Reads the {@code Build-Revision} attribute from the MANIFEST.MF of the
JAR that contains this class.
+ * Returns null when running outside a packaged JAR (e.g., during {@code mvn
test}).
+ */
+ protected String getBuildRevision()
+ {
+ try {
+ URL classUrl =
EmitterModule.class.getResource(EmitterModule.class.getSimpleName() + ".class");
+ if (classUrl != null && "jar".equals(classUrl.getProtocol())) {
+ String classPath = classUrl.toString();
+ String manifestPath = classPath.substring(0,
classPath.lastIndexOf('!') + 1) + "/META-INF/MANIFEST.MF";
+ try (InputStream is = new URL(manifestPath).openStream()) {
+ return new
Manifest(is).getMainAttributes().getValue("Build-Revision");
+ }
+ }
+ }
+ catch (IOException e) {
+ // Fall through and return null
Review Comment:
Log the exception so it doesn't mask a legitimate issue
##########
docs/querying/sql-metadata-tables.md:
##########
@@ -237,6 +237,7 @@ Servers table lists all discovered servers in the cluster.
|is_leader|BIGINT|1 if the server is currently the 'leader' (for services
which have the concept of leadership), otherwise 0 if the server is not the
leader, or null if the server type does not have the concept of leadership|
|start_time|STRING|Timestamp in ISO8601 format when the server was announced
in the cluster|
|version|VARCHAR|Druid version running on the server|
+|build_revision|VARCHAR|The git commit of the build that produced the server
binary. Empty string when running outside a packaged binary (e.g., during `mvn
test`)|
Review Comment:
Optionally we could also wire this to the console's services view by default
in web-`console/src/views/services-view/services-view.tsx` and
`web-console/src/views/services-view/__snapshots__/services-view.spec.tsx.snap`:
https://github.com/apache/druid/pull/18542/changes#diff-07dc2b09eeb1fc84b83d5ec80b95e4cda9fb235200e26b37271cc73d8d62b883
##########
server/src/main/java/org/apache/druid/server/emitter/EmitterModule.java:
##########
@@ -177,4 +184,26 @@ public Emitter get()
return emitter;
}
}
+
+ /**
+ * Reads the {@code Build-Revision} attribute from the MANIFEST.MF of the
JAR that contains this class.
+ * Returns null when running outside a packaged JAR (e.g., during {@code mvn
test}).
+ */
+ protected String getBuildRevision()
+ {
+ try {
+ URL classUrl =
EmitterModule.class.getResource(EmitterModule.class.getSimpleName() + ".class");
+ if (classUrl != null && "jar".equals(classUrl.getProtocol())) {
+ String classPath = classUrl.toString();
+ String manifestPath = classPath.substring(0,
classPath.lastIndexOf('!') + 1) + "/META-INF/MANIFEST.MF";
+ try (InputStream is = new URL(manifestPath).openStream()) {
+ return new
Manifest(is).getMainAttributes().getValue("Build-Revision");
+ }
+ }
+ }
+ catch (IOException e) {
+ // Fall through and return null
+ }
+ return null;
Review Comment:
Since both `EmitterModule` and `DruidNode` converts null to empty with
`nullToEmptyNonDruidDataString()`, I think it'd be good to have this method
directly return `""` (for tests)
##########
server/src/main/java/org/apache/druid/server/DruidNode.java:
##########
@@ -266,6 +275,33 @@ public String getVersion()
return version;
}
+ public String getBuildRevision()
+ {
+ return buildRevision;
+ }
+
+ /**
+ * Reads the {@code Build-Revision} attribute from the MANIFEST.MF of the
JAR containing this class.
+ * Returns null when running outside a packaged JAR (e.g., during {@code mvn
test}).
+ */
+ private static String readBuildRevisionFromManifest()
+ {
+ try {
+ URL classUrl =
DruidNode.class.getResource(DruidNode.class.getSimpleName() + ".class");
+ if (classUrl != null && "jar".equals(classUrl.getProtocol())) {
+ String classPath = classUrl.toString();
+ String manifestPath = classPath.substring(0,
classPath.lastIndexOf('!') + 1) + "/META-INF/MANIFEST.MF";
+ try (InputStream is = new URL(manifestPath).openStream()) {
+ return new
Manifest(is).getMainAttributes().getValue("Build-Revision");
+ }
+ }
+ }
+ catch (IOException e) {
+ // Fall through and return null
+ }
+ return null;
+ }
Review Comment:
We can create a class `BuildInfo` and move this utility to it so both
EmitterModule and this code can share it.
##########
docs/querying/sql-metadata-tables.md:
##########
@@ -237,6 +237,7 @@ Servers table lists all discovered servers in the cluster.
|is_leader|BIGINT|1 if the server is currently the 'leader' (for services
which have the concept of leadership), otherwise 0 if the server is not the
leader, or null if the server type does not have the concept of leadership|
|start_time|STRING|Timestamp in ISO8601 format when the server was announced
in the cluster|
|version|VARCHAR|Druid version running on the server|
+|build_revision|VARCHAR|The git commit of the build that produced the server
binary. Empty string when running outside a packaged binary (e.g., during `mvn
test`)|
Review Comment:
```suggestion
|build_revision|VARCHAR|The git commit of the build that produced the server
binary.|
```
##########
pom.xml:
##########
@@ -1885,7 +1885,7 @@
</execution>
</executions>
<configuration>
- <dotGitDirectory>${project.basedir}/.git</dotGitDirectory>
+
<dotGitDirectory>${session.executionRootDirectory}/.git</dotGitDirectory>
Review Comment:
Is this change needed? Or will it still work without this multi-module
setting?
##########
docs/operations/metrics.md:
##########
@@ -31,6 +31,8 @@ All Druid metrics share a common set of fields:
* `metric`: the name of the metric
* `service`: the service name that emitted the metric
* `host`: the host name that emitted the metric
+* `version`: the Druid version of the service that emitted the metric
+* `buildRevision`: the git commit of the build that produced the service
binary. Useful for verifying that all nodes in a cluster are running the
intended revision during rolling deployments. Empty string when running outside
a packaged binary (e.g., during `mvn test`).
Review Comment:
These are user-facing docs, so I think we can exclude the note about `mvn
test` that's already captured in the javadocs for devs:
```suggestion
* `buildRevision`: the git commit of the build that produced the service
binary. Useful for verifying that all nodes in a cluster are running the
intended revision during rolling deployments.
```
--
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]
