You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "suneet-s (via GitHub)" <gi...@apache.org> on 2023/05/02 16:04:50 UTC
[GitHub] [druid] suneet-s opened a new pull request, #14200: Add the ability to optionally bind extra service dimensions
suneet-s opened a new pull request, #14200:
URL: https://github.com/apache/druid/pull/14200
### Description
This change makes it so that extension writers can now optionally bind extra service dimensions. This is useful if an extension wants to bind service dimensions that are only relevant to one node type, but not the others. This will allow the extension to be listed in the extension load list in the common runtime properties without needing to inject invalid properties for all the other services where the dimensions are not relevant.
Developer facing docs are added to the `ExtraServiceDimensions` class.
#### Release note
NEW: Extension developers can optionally bind extra service dimensions to be emitted by the service emitter.
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [x] added documentation for new or modified features or behaviors.
- [x] a release note entry in the PR description.
- [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a diff in pull request #14200: Ability to optionally bind extra service dimensions to ServiceEvents
Posted by "TSFenwick (via GitHub)" <gi...@apache.org>.
TSFenwick commented on code in PR #14200:
URL: https://github.com/apache/druid/pull/14200#discussion_r1183109745
##########
server/src/main/java/org/apache/druid/server/emitter/EmitterModule.java:
##########
@@ -82,33 +83,49 @@ public void configure(Binder binder)
binder.bind(Emitter.class).toProvider(new EmitterProvider(emitterType)).in(LazySingleton.class);
- MapBinder<String, String> extraServiceDimensions = MapBinder.newMapBinder(
+ MapBinder<String, String> extraServiceDimensionsLegacy = MapBinder.newMapBinder(
Review Comment:
nit: maybe requiredExtraServiceDimension or leave as is and other one is optionalExtraServiceDimensions...
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a diff in pull request #14200: Ability to optionally bind extra service dimensions to ServiceEvents
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on code in PR #14200:
URL: https://github.com/apache/druid/pull/14200#discussion_r1195598954
##########
server/src/main/java/org/apache/druid/server/emitter/ExtraServiceDimensions.java:
##########
@@ -29,12 +29,25 @@
/**
* Annotation to inject extra dimensions, added to all events, emitted via {@link EmitterModule#getServiceEmitter}.
- *
+ * <p/>
* For example, write this in a body of {@link com.google.inject.Module#configure} of your extension module):
- *
+ * <p/>
* MapBinder<String, String> extraDims =
* MapBinder.newMapBinder(binder, String.class, String.class, ExtraServiceDimensions.class);
* extraDims.addBinding("foo").toInstance("bar");
+ * <p/>
+ * If a module wishes to optionally bind service dimensions they may do so by using the binding to
+ * Map<String, Optional<String>. The key is only added to the service dimensions that are emitted if the Optional is
+ * present.
+ * <p/>
+ * MapBinder<String, Optional<String>> extraDims =
+ * MapBinder.newMapBinder(
+ * binder,
+ * new TypeLiteral<String>() {},
+ * new TypeLiteral<Optional<String>>() {},
+ * ExtraServiceDimensions.class
+ * );
+ * extraDims.addBinding("foo").toInstance(Optional.fromNullable(bar));
Review Comment:
Thanks for the pointer @TSFenwick! Since that works, I will close this PR. There's no need to introduce another way of binding these dimensions
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a diff in pull request #14200: Ability to optionally bind extra service dimensions to ServiceEvents
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on code in PR #14200:
URL: https://github.com/apache/druid/pull/14200#discussion_r1184107416
##########
server/src/main/java/org/apache/druid/server/emitter/ExtraServiceDimensions.java:
##########
@@ -29,12 +29,25 @@
/**
* Annotation to inject extra dimensions, added to all events, emitted via {@link EmitterModule#getServiceEmitter}.
- *
+ * <p/>
* For example, write this in a body of {@link com.google.inject.Module#configure} of your extension module):
- *
+ * <p/>
* MapBinder<String, String> extraDims =
* MapBinder.newMapBinder(binder, String.class, String.class, ExtraServiceDimensions.class);
* extraDims.addBinding("foo").toInstance("bar");
+ * <p/>
+ * If a module wishes to optionally bind service dimensions they may do so by using the binding to
+ * Map<String, Optional<String>. The key is only added to the service dimensions that are emitted if the Optional is
+ * present.
+ * <p/>
+ * MapBinder<String, Optional<String>> extraDims =
+ * MapBinder.newMapBinder(
+ * binder,
+ * new TypeLiteral<String>() {},
+ * new TypeLiteral<Optional<String>>() {},
+ * ExtraServiceDimensions.class
+ * );
+ * extraDims.addBinding("foo").toInstance(Optional.fromNullable(bar));
Review Comment:
If you want to optionally bind the key to something that depends on an injected value, you can not with the `String` -> `String` mapping. This is because the binding needs to be done in the configure method, while you can only look at the injected value after the configure method of the module, for example in a Provider.
MapBinder does not allow binding to null, so you can't use a Provider that returns null to indicate that you do not want the key to be bound. https://github.com/google/guice/wiki/NULL_VALUE_IN_MAP#using-null-as-absent-value
Perhaps a better example that explains the usefulness of this is
```
extraDims.addBinding("foo").toProvider(
new Provider<Optional<String>()
{
@Inject
private DruidConfig config;
@Override
public Optional<String> get()
{
return config.isAdditionalServiceDimensionsEnabled() ? Optional.of(config.getFoo()) : Optional.empty();
}
}
);
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a diff in pull request #14200: Ability to optionally bind extra service dimensions to ServiceEvents
Posted by "TSFenwick (via GitHub)" <gi...@apache.org>.
TSFenwick commented on code in PR #14200:
URL: https://github.com/apache/druid/pull/14200#discussion_r1194411381
##########
server/src/main/java/org/apache/druid/server/emitter/ExtraServiceDimensions.java:
##########
@@ -29,12 +29,25 @@
/**
* Annotation to inject extra dimensions, added to all events, emitted via {@link EmitterModule#getServiceEmitter}.
- *
+ * <p/>
* For example, write this in a body of {@link com.google.inject.Module#configure} of your extension module):
- *
+ * <p/>
* MapBinder<String, String> extraDims =
* MapBinder.newMapBinder(binder, String.class, String.class, ExtraServiceDimensions.class);
* extraDims.addBinding("foo").toInstance("bar");
+ * <p/>
+ * If a module wishes to optionally bind service dimensions they may do so by using the binding to
+ * Map<String, Optional<String>. The key is only added to the service dimensions that are emitted if the Optional is
+ * present.
+ * <p/>
+ * MapBinder<String, Optional<String>> extraDims =
+ * MapBinder.newMapBinder(
+ * binder,
+ * new TypeLiteral<String>() {},
+ * new TypeLiteral<Optional<String>>() {},
+ * ExtraServiceDimensions.class
+ * );
+ * extraDims.addBinding("foo").toInstance(Optional.fromNullable(bar));
Review Comment:
Looks like its possible to inject the node roles into the module class. meaning we dont need the optional
```java
private Set<NodeRole> nodeRoles;
@Inject
public void setNodeRoles(@Self Set<NodeRole> nodeRoles)
{
this.nodeRoles = nodeRoles;
}
@Override
public void configure(Binder binder)
{
MapBinder<String, String> extraDims = MapBinder.newMapBinder(
binder,
String.class,
String.class,
ExtraServiceDimensions.class
);
// add information related to tasks for peons
if (nodeRoles.contains(NodeRole.PEON)) {
extraDims.addBinding("foo").toProvider(new Provider<String>()
{
@Inject
private Injector injector;
@Override
public String get()
{
return injector.getInstance(Task.class).getId();
}
});
}
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] TSFenwick commented on a diff in pull request #14200: Ability to optionally bind extra service dimensions to ServiceEvents
Posted by "TSFenwick (via GitHub)" <gi...@apache.org>.
TSFenwick commented on code in PR #14200:
URL: https://github.com/apache/druid/pull/14200#discussion_r1183109745
##########
server/src/main/java/org/apache/druid/server/emitter/EmitterModule.java:
##########
@@ -82,33 +83,49 @@ public void configure(Binder binder)
binder.bind(Emitter.class).toProvider(new EmitterProvider(emitterType)).in(LazySingleton.class);
- MapBinder<String, String> extraServiceDimensions = MapBinder.newMapBinder(
+ MapBinder<String, String> extraServiceDimensionsLegacy = MapBinder.newMapBinder(
Review Comment:
maybe requiredExtraServiceDimension or leave as is and other one is optionalExtraServiceDimensions...
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s closed pull request #14200: Ability to optionally bind extra service dimensions to ServiceEvents
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s closed pull request #14200: Ability to optionally bind extra service dimensions to ServiceEvents
URL: https://github.com/apache/druid/pull/14200
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #14200: Add the ability to optionally bind extra service dimensions
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on PR #14200:
URL: https://github.com/apache/druid/pull/14200#issuecomment-1531741639
Added `Design Review` because it updates the contract of `ExtraServiceDimensions`
No disruption is expected for extensions that already use the ExtraServiceDimensions bindings.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #14200: Ability to optionally bind extra service dimensions to ServiceEvents
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #14200:
URL: https://github.com/apache/druid/pull/14200#discussion_r1183225680
##########
server/src/main/java/org/apache/druid/server/emitter/ExtraServiceDimensions.java:
##########
@@ -29,12 +29,25 @@
/**
* Annotation to inject extra dimensions, added to all events, emitted via {@link EmitterModule#getServiceEmitter}.
- *
+ * <p/>
* For example, write this in a body of {@link com.google.inject.Module#configure} of your extension module):
- *
+ * <p/>
* MapBinder<String, String> extraDims =
* MapBinder.newMapBinder(binder, String.class, String.class, ExtraServiceDimensions.class);
* extraDims.addBinding("foo").toInstance("bar");
+ * <p/>
+ * If a module wishes to optionally bind service dimensions they may do so by using the binding to
+ * Map<String, Optional<String>. The key is only added to the service dimensions that are emitted if the Optional is
+ * present.
+ * <p/>
+ * MapBinder<String, Optional<String>> extraDims =
+ * MapBinder.newMapBinder(
+ * binder,
+ * new TypeLiteral<String>() {},
+ * new TypeLiteral<Optional<String>>() {},
+ * ExtraServiceDimensions.class
+ * );
+ * extraDims.addBinding("foo").toInstance(Optional.fromNullable(bar));
Review Comment:
What's the advantage of doing this, vs. keeping `String` -> `String` and having modules avoid calling `addBinding` if `bar` is null?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a diff in pull request #14200: Ability to optionally bind extra service dimensions to ServiceEvents
Posted by "suneet-s (via GitHub)" <gi...@apache.org>.
suneet-s commented on code in PR #14200:
URL: https://github.com/apache/druid/pull/14200#discussion_r1184107416
##########
server/src/main/java/org/apache/druid/server/emitter/ExtraServiceDimensions.java:
##########
@@ -29,12 +29,25 @@
/**
* Annotation to inject extra dimensions, added to all events, emitted via {@link EmitterModule#getServiceEmitter}.
- *
+ * <p/>
* For example, write this in a body of {@link com.google.inject.Module#configure} of your extension module):
- *
+ * <p/>
* MapBinder<String, String> extraDims =
* MapBinder.newMapBinder(binder, String.class, String.class, ExtraServiceDimensions.class);
* extraDims.addBinding("foo").toInstance("bar");
+ * <p/>
+ * If a module wishes to optionally bind service dimensions they may do so by using the binding to
+ * Map<String, Optional<String>. The key is only added to the service dimensions that are emitted if the Optional is
+ * present.
+ * <p/>
+ * MapBinder<String, Optional<String>> extraDims =
+ * MapBinder.newMapBinder(
+ * binder,
+ * new TypeLiteral<String>() {},
+ * new TypeLiteral<Optional<String>>() {},
+ * ExtraServiceDimensions.class
+ * );
+ * extraDims.addBinding("foo").toInstance(Optional.fromNullable(bar));
Review Comment:
@gianm If you want to optionally bind the key to something that depends on an injected value, you can not with the `String` -> `String` mapping. This is because the binding needs to be done in the configure method, while you can only look at the injected value after the configure method of the module, for example in a Provider.
MapBinder does not allow binding to null, so you can't use a Provider that returns null to indicate that you do not want the key to be bound. https://github.com/google/guice/wiki/NULL_VALUE_IN_MAP#using-null-as-absent-value
Perhaps a better example that explains the usefulness of this is
```
extraDims.addBinding("foo").toProvider(
new Provider<Optional<String>()
{
@Inject
private DruidConfig config;
@Override
public Optional<String> get()
{
return config.isAdditionalServiceDimensionsEnabled() ? Optional.of(config.getFoo()) : Optional.empty();
}
}
);
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #14200: Ability to optionally bind extra service dimensions to ServiceEvents
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #14200:
URL: https://github.com/apache/druid/pull/14200#discussion_r1187849317
##########
server/src/main/java/org/apache/druid/server/emitter/ExtraServiceDimensions.java:
##########
@@ -29,12 +29,25 @@
/**
* Annotation to inject extra dimensions, added to all events, emitted via {@link EmitterModule#getServiceEmitter}.
- *
+ * <p/>
* For example, write this in a body of {@link com.google.inject.Module#configure} of your extension module):
- *
+ * <p/>
* MapBinder<String, String> extraDims =
* MapBinder.newMapBinder(binder, String.class, String.class, ExtraServiceDimensions.class);
* extraDims.addBinding("foo").toInstance("bar");
+ * <p/>
+ * If a module wishes to optionally bind service dimensions they may do so by using the binding to
+ * Map<String, Optional<String>. The key is only added to the service dimensions that are emitted if the Optional is
+ * present.
+ * <p/>
+ * MapBinder<String, Optional<String>> extraDims =
+ * MapBinder.newMapBinder(
+ * binder,
+ * new TypeLiteral<String>() {},
+ * new TypeLiteral<Optional<String>>() {},
+ * ExtraServiceDimensions.class
+ * );
+ * extraDims.addBinding("foo").toInstance(Optional.fromNullable(bar));
Review Comment:
Got it, that makes sense. Thanks for illuminating it.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org