jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r775046521
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -63,6 +67,26 @@ public ZkWorker(Worker worker, PathChildrenCache
statusCache, final ObjectMapper
this.statusCache = statusCache;
this.cacheConverter = (ChildData input) ->
JacksonUtils.readValue(jsonMapper, input.getData(),
TaskAnnouncement.class);
+ this.taskIdExtractor = createTaskIdExtractor(jsonMapper);
+ }
+
+ static java.util.function.Function<ChildData, String>
createTaskIdExtractor(final ObjectMapper jsonMapper)
+ {
+ return (ChildData input) -> {
+ try {
+ JsonParser parser =
jsonMapper.getFactory().createParser(input.getData());
+ while (parser.nextToken() != JsonToken.END_OBJECT) {
+ if ("id".equals(parser.getCurrentName())) {
+ parser.nextToken();
+ return parser.getText();
+ }
+ }
+ return null;
Review comment:
It will not result in an exception in `getRunningTaskIds`, but it would
allow `null` to be in the result set. I think from this class perspective is
valid (if a task exists with a null id, then `getRunningTaskIds` should include
null). And, `TaskAnnouncement` currently would (should?) exhibit this behaviour
on deserialization.
However, I am not clear on the right behaviour here. If all tasks should
have valid Ids, then I think the clearer least-surprising behaviour is to
explicitly throw an exception, or maybe filter `null` elements out of the
return value from `getRunningTaskIds`.
What do you think?
--
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]
