utafrali commented on code in PR #19350:
URL: https://github.com/apache/druid/pull/19350#discussion_r3104072024
##########
processing/src/main/java/org/apache/druid/java/util/emitter/service/ServiceMetricEvent.java:
##########
@@ -93,6 +93,11 @@ public String getHost()
return serviceDims.get(HOST);
}
+ public ImmutableMap<String, String> getServiceDims()
Review Comment:
The return type `ImmutableMap<String, String>` leaks the concrete
implementation, which is inconsistent with `getUserDims()` in the same class —
that method returns `Map<String, Object>` even though its backing field is also
an `ImmutableMap`. For a public API, prefer returning the interface type:
```java
public Map<String, String> getServiceDims()
{
return serviceDims;
}
```
Callers who need the immutability guarantee can check `instanceof
ImmutableMap` or the Javadoc can document it. Returning the concrete type here
makes it harder to swap the implementation later.
##########
processing/src/test/java/org/apache/druid/java/util/emitter/service/ServiceMetricEventTest.java:
##########
@@ -76,6 +76,9 @@ public void testBuilder()
builderEvent.toMap()
);
+ Assert.assertEquals("test", builderEvent.getServiceDims().get("service"));
Review Comment:
The string literals `"service"` and `"host"` duplicate the key constants
already defined in the class (used inside `getService()` and `getHost()`). If
those constants (`SERVICE`, `HOST`) are accessible from the test, use them
directly to avoid silent drift if the keys ever change:
```java
Assert.assertEquals("test",
builderEvent.getServiceDims().get(ServiceMetricEvent.SERVICE));
Assert.assertEquals("localhost",
builderEvent.getServiceDims().get(ServiceMetricEvent.HOST));
```
If the constants are private, at minimum add a comment explaining why
literals are used here.
--
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]
