You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/12/23 21:00:19 UTC
[GitHub] [druid] jasonk000 opened a new pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
jasonk000 opened a new pull request #12096:
URL: https://github.com/apache/druid/pull/12096
### Description
Improve the performance of `RemoteTaskRunner::tryAssignTask` which consumes long periods of CPU on the Overlord during a task restart operation.
Screenshot of profiler showing long period of `rtr-pending-..` task thread.
Screenshot of profile flamegraph for this thread, showing 100pc of CPU in `tryAssignTask` loop:
##### Key changed/added classes in this PR
This change:
1. eliminates triple nested call of `getRunningTasks()` in `ZkWorker::toImmutable`, and,
2. reduces the work performed in `ZkWorker::isRunningTask` by parsing only the `id` field instead of the entire ZkWorker json.
By eliminating this extra work, the loop is much tighter.
This is a change coupled to this mailing thread discussion:
https://lists.apache.org/thread/9jgdwrodwsfcg98so6kzfhdmn95gzyrj
##### Tests
Tests in `RemoteTaskRunner*Test.java` capture this functionality.
<hr>
This PR has:
- [x] been self-reviewed.
- [x] been tested in a test Druid cluster (as a part of a larger block of changes).
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r779951436
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/worker/TaskAnnouncement.java
##########
@@ -35,6 +35,8 @@
*/
public class TaskAnnouncement
{
+ public static final String TASK_ID_KEY = "id";
Review comment:
maybe we can make this more clear by including a `static String getTaskIdKeyName()` { return "id"; }`, which can be called from `ZkWorker`, thoughts?
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r774855761
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -63,6 +66,27 @@ 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 {
+ JsonNode parent = jsonMapper.readTree(input.getData());
Review comment:
FWIW this fix done properly converts this (the block on the right-hand side) ...
to this ...
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r779965080
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/worker/TaskAnnouncement.java
##########
@@ -35,6 +35,8 @@
*/
public class TaskAnnouncement
{
+ public static final String TASK_ID_KEY = "id";
Review comment:
Oh, I see the confusion; I created it here but didn't land it in the PR. Will include in fix.
--
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] samarthjain commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
samarthjain commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r814536835
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -102,19 +136,29 @@ public Worker getWorker()
@JsonProperty("currCapacityUsed")
public int getCurrCapacityUsed()
+ {
+ return getCurrCapacityUsed(getRunningTasks());
+ }
+
+ static int getCurrCapacityUsed(Map<String, TaskAnnouncement> tasks)
Review comment:
This could be private.
##########
File path: indexing-service/src/test/java/org/apache/druid/indexing/overlord/ZkWorkerTest.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.indexing.overlord;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.curator.framework.recipes.cache.ChildData;
+import org.apache.druid.jackson.DefaultObjectMapper;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.zookeeper.data.Stat;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.function.Function;
+
+public class ZkWorkerTest
+{
+ Function<ChildData, String> extract;
+
+ @Before
+ public void setup()
+ {
+ ObjectMapper mapper = new DefaultObjectMapper();
+ extract = ZkWorker.createTaskIdExtractor(mapper);
+ }
+
+ ChildData prepare(String input)
+ {
+ String replaced = StringUtils.replaceChar(input, '\'', "\"");
+ byte[] data = StringUtils.toUtf8(replaced);
+ return new ChildData("/a/b/c", new Stat(), data);
+ }
+
+ @Test
+ public void testShallowObjectWithIdFirst()
+ {
+ ChildData input = prepare("{'id': 'abcd', 'status': 'RUNNING'}");
Review comment:
Minor point - it would be nice to use `TaskAnnouncment. TASK_ID_KEY` in place of `id` here and other tests.
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -102,19 +136,29 @@ public Worker getWorker()
@JsonProperty("currCapacityUsed")
public int getCurrCapacityUsed()
+ {
+ return getCurrCapacityUsed(getRunningTasks());
+ }
+
+ static int getCurrCapacityUsed(Map<String, TaskAnnouncement> tasks)
{
int currCapacity = 0;
- for (TaskAnnouncement taskAnnouncement : getRunningTasks().values()) {
+ for (TaskAnnouncement taskAnnouncement : tasks.values()) {
currCapacity += taskAnnouncement.getTaskResource().getRequiredCapacity();
}
return currCapacity;
}
@JsonProperty("availabilityGroups")
public Set<String> getAvailabilityGroups()
+ {
+ return getAvailabilityGroups(getRunningTasks());
+ }
+
+ static Set<String> getAvailabilityGroups(Map<String, TaskAnnouncement> tasks)
Review comment:
This could be private as well.
--
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] samarthjain commented on pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
samarthjain commented on pull request #12096:
URL: https://github.com/apache/druid/pull/12096#issuecomment-1057197759
+1. Thank you for your work, @jasonk000 .
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r774855761
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -63,6 +66,27 @@ 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 {
+ JsonNode parent = jsonMapper.readTree(input.getData());
Review comment:
FWIW this fix done properly converts this (note in particular the block on the right-hand edge shrinks significantly) ...
to this ...
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r774855761
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -63,6 +66,27 @@ 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 {
+ JsonNode parent = jsonMapper.readTree(input.getData());
Review comment:
FWIW this fix done properly converts this (note in particular the block on the right-hand edge) ...
to this ...
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
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: 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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r779950249
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/worker/TaskAnnouncement.java
##########
@@ -35,6 +35,8 @@
*/
public class TaskAnnouncement
{
+ public static final String TASK_ID_KEY = "id";
Review comment:
In my view it's closer to being in the 'model' aka in the `TaskAnnouncement` record, since we want the `taskId`. It should also act as a prompt so that if the `TaskAnnouncement` field is changed at any point then compiler will flag an issue.
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r779951797
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -63,6 +67,25 @@ 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) {
Review comment:
Likely, although this hasn't been an issue in my experience, it could become one;
I'll add tests and improve code to match.
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r779951436
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/worker/TaskAnnouncement.java
##########
@@ -35,6 +35,8 @@
*/
public class TaskAnnouncement
{
+ public static final String TASK_ID_KEY = "id";
Review comment:
maybe we can make this more clear by including a `static String getTaskIdKeyName() { return "id"; }`, which can be called from `ZkWorker`, thoughts?
--
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] FrankChen021 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r774930619
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -73,19 +74,18 @@ public ZkWorker(Worker worker, PathChildrenCache statusCache, final ObjectMapper
{
return (ChildData input) -> {
try {
- JsonNode parent = jsonMapper.readTree(input.getData());
- if (parent != null) {
- JsonNode id = parent.get(TaskAnnouncement.TASK_ID_KEY);
- if (id != null) {
- return id.asText();
+ JsonParser parser = jsonMapper.getFactory().createParser(input.getData());
Review comment:
Use try-with-resources statement to ensure parser is closed.
try (JsonParser parser = jsonMapper.getFactory().createParser(input.getData())) {
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r779950249
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/worker/TaskAnnouncement.java
##########
@@ -35,6 +35,8 @@
*/
public class TaskAnnouncement
{
+ public static final String TASK_ID_KEY = "id";
Review comment:
In my view it's closer to being in the 'model' aka in the `TaskAnnouncement` record, since we want the `taskId`. It should also act as a prompt so that if the `TaskAnnouncement field is renamed at any point then compiler will flag an issue.
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r817260355
##########
File path: indexing-service/src/test/java/org/apache/druid/indexing/overlord/ZkWorkerTest.java
##########
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.indexing.overlord;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.curator.framework.recipes.cache.ChildData;
+import org.apache.druid.indexing.worker.TaskAnnouncement;
+import org.apache.druid.jackson.DefaultObjectMapper;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.zookeeper.data.Stat;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.function.Function;
+
+public class ZkWorkerTest
Review comment:
added test in 9a2175dc4
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r775047335
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -73,19 +74,18 @@ public ZkWorker(Worker worker, PathChildrenCache statusCache, final ObjectMapper
{
return (ChildData input) -> {
try {
- JsonNode parent = jsonMapper.readTree(input.getData());
- if (parent != null) {
- JsonNode id = parent.get(TaskAnnouncement.TASK_ID_KEY);
- if (id != null) {
- return id.asText();
+ JsonParser parser = jsonMapper.getFactory().createParser(input.getData());
Review comment:
Fixed in 2360cd3963
--
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] xvrl commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r779798267
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -63,6 +67,25 @@ 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) {
Review comment:
wouldn't this stop parsing on the end of the first nested object if the ordering of the fields is not guaranteed to have "id" come first?
--
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] FrankChen021 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r774835897
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -63,6 +66,27 @@ 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 {
+ JsonNode parent = jsonMapper.readTree(input.getData());
Review comment:
`readTree` still deserializes all the input to object. Since here we just want to get the value of `id` field, I think we can use jackson's streaming parser api to get the id so that we don't need to parse all the string content.
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r774847963
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -63,6 +66,27 @@ 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 {
+ JsonNode parent = jsonMapper.readTree(input.getData());
Review comment:
You're totally right. Let me work on this. :facepalm: ..
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
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 system-wide 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: 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] samarthjain commented on pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
samarthjain commented on pull request #12096:
URL: https://github.com/apache/druid/pull/12096#issuecomment-1054602169
> HI @samarthjain @xvrl , I believe these comments have all been addressed in [a633c74](https://github.com/apache/druid/pull/12096/commits/a633c7447e69f407b58140b61f1d04ef6d078da4). I included a set of unit tests to confirm that any particular structure should be correctly read, so, I think this is good to go?
Hi, @jasonk000 - sorry, it looks like I forgot publishing my review comments. They are mostly minor.
--
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] samarthjain commented on pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
samarthjain commented on pull request #12096:
URL: https://github.com/apache/druid/pull/12096#issuecomment-1050627364
Some minor comments, otherwise looks good.
+1 after the above are addressed and CI passes.
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r779950249
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/worker/TaskAnnouncement.java
##########
@@ -35,6 +35,8 @@
*/
public class TaskAnnouncement
{
+ public static final String TASK_ID_KEY = "id";
Review comment:
~In my view it's closer to being in the 'model' aka in the `TaskAnnouncement` record, since we want the `taskId`. It should also act as a prompt so that if the `TaskAnnouncement` field is changed at any point then compiler will flag an issue.~
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/worker/TaskAnnouncement.java
##########
@@ -35,6 +35,8 @@
*/
public class TaskAnnouncement
{
+ public static final String TASK_ID_KEY = "id";
Review comment:
~maybe we can make this more clear by including a `static String getTaskIdKeyName() { return "id"; }`, which can be called from `ZkWorker`, thoughts?~
--
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] FrankChen021 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r774931114
##########
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:
what if a `null` is returned? will it result in exception in the `getRunningTaskIds`?
--
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] FrankChen021 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r775120176
##########
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:
I don't know if there's a case that there's no `id` field in the task. Maybe it only exists in our code here in theory. Since here is a performance critical path based on your investigation, I think we can leave the null 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: 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] xvrl commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r779802080
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/worker/TaskAnnouncement.java
##########
@@ -35,6 +35,8 @@
*/
public class TaskAnnouncement
{
+ public static final String TASK_ID_KEY = "id";
Review comment:
did you mean to use this constant in the streaming parser?
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r779950690
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -63,6 +67,25 @@ 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)
Review comment:
this is covered in ZkWorker, however, some of the negative / problematic cases you bring up in other comments do warrant specific tests for this; so i'll add some tests and resolve any issues
--
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] xvrl commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r779800261
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -63,6 +67,25 @@ 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())) {
Review comment:
it would probably be worthwhile to put some safeguards to ensure we don't accidentally extract an "id" field from one of the nested objects
--
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] xvrl commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r779972037
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -63,6 +67,25 @@ 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) {
Review comment:
I don't think we offer any ordering guarantees today in the serialization, so this could break without warning
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r816281880
##########
File path: indexing-service/src/test/java/org/apache/druid/indexing/overlord/ZkWorkerTest.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.indexing.overlord;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.curator.framework.recipes.cache.ChildData;
+import org.apache.druid.jackson.DefaultObjectMapper;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.zookeeper.data.Stat;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.function.Function;
+
+public class ZkWorkerTest
+{
+ Function<ChildData, String> extract;
+
+ @Before
+ public void setup()
+ {
+ ObjectMapper mapper = new DefaultObjectMapper();
+ extract = ZkWorker.createTaskIdExtractor(mapper);
+ }
+
+ ChildData prepare(String input)
+ {
+ String replaced = StringUtils.replaceChar(input, '\'', "\"");
+ byte[] data = StringUtils.toUtf8(replaced);
+ return new ChildData("/a/b/c", new Stat(), data);
+ }
+
+ @Test
+ public void testShallowObjectWithIdFirst()
+ {
+ ChildData input = prepare("{'id': 'abcd', 'status': 'RUNNING'}");
Review comment:
fixed in https://github.com/apache/druid/pull/12096/commits/6603e32c1061f929e5b30df2f504965a982b5606
--
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] xvrl commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r817180157
##########
File path: indexing-service/src/test/java/org/apache/druid/indexing/overlord/ZkWorkerTest.java
##########
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.indexing.overlord;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.curator.framework.recipes.cache.ChildData;
+import org.apache.druid.indexing.worker.TaskAnnouncement;
+import org.apache.druid.jackson.DefaultObjectMapper;
+import org.apache.druid.java.util.common.StringUtils;
+import org.apache.zookeeper.data.Stat;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.function.Function;
+
+public class ZkWorkerTest
Review comment:
can we add a test where we serialize the task info using jackson and then deserialize with the task id extractor? this will ensure we update this code whenever the task serialization format changes.
--
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] samarthjain merged pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
samarthjain merged pull request #12096:
URL: https://github.com/apache/druid/pull/12096
--
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] jasonk000 commented on pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on pull request #12096:
URL: https://github.com/apache/druid/pull/12096#issuecomment-1054591159
HI @samarthjain @xvrl , I believe these comments have all been addressed in [a633c74](https://github.com/apache/druid/pull/12096/commits/a633c7447e69f407b58140b61f1d04ef6d078da4). I included a set of unit tests to confirm that any particular structure should be correctly read, so, I think this is good to go?
--
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] jasonk000 commented on pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on pull request #12096:
URL: https://github.com/apache/druid/pull/12096#issuecomment-1055793168
@samarthjain squashed & rebased
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r816281705
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -102,19 +136,29 @@ public Worker getWorker()
@JsonProperty("currCapacityUsed")
public int getCurrCapacityUsed()
+ {
+ return getCurrCapacityUsed(getRunningTasks());
+ }
+
+ static int getCurrCapacityUsed(Map<String, TaskAnnouncement> tasks)
{
int currCapacity = 0;
- for (TaskAnnouncement taskAnnouncement : getRunningTasks().values()) {
+ for (TaskAnnouncement taskAnnouncement : tasks.values()) {
currCapacity += taskAnnouncement.getTaskResource().getRequiredCapacity();
}
return currCapacity;
}
@JsonProperty("availabilityGroups")
public Set<String> getAvailabilityGroups()
+ {
+ return getAvailabilityGroups(getRunningTasks());
+ }
+
+ static Set<String> getAvailabilityGroups(Map<String, TaskAnnouncement> tasks)
Review comment:
fixed in https://github.com/apache/druid/pull/12096/commits/b9b13195c4c640ceaf73d18f5fea355bf86247fd
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -102,19 +136,29 @@ public Worker getWorker()
@JsonProperty("currCapacityUsed")
public int getCurrCapacityUsed()
+ {
+ return getCurrCapacityUsed(getRunningTasks());
+ }
+
+ static int getCurrCapacityUsed(Map<String, TaskAnnouncement> tasks)
Review comment:
fixed in https://github.com/apache/druid/pull/12096/commits/b9b13195c4c640ceaf73d18f5fea355bf86247fd
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r774855442
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -63,6 +66,27 @@ 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 {
+ JsonNode parent = jsonMapper.readTree(input.getData());
Review comment:
OK, captured this in d5a9864c8d8f79e7b8235321d9f33402206f568d. Thanks for feedback.
--
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] FrankChen021 commented on pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
FrankChen021 commented on pull request #12096:
URL: https://github.com/apache/druid/pull/12096#issuecomment-1000605264
Good catch. Overall LGTM. Leave a comment to you to see if we can improve more.
--
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] xvrl commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
xvrl commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r779792677
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -63,6 +67,25 @@ 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)
Review comment:
can we add some tests for this method to understand the expected behavior? This would also ensure it's less likely to break if new fields are added to the task data.
--
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] jasonk000 commented on a change in pull request #12096: perf: improve RemoteTaskRunner task assignment loop performance
Posted by GitBox <gi...@apache.org>.
jasonk000 commented on a change in pull request #12096:
URL: https://github.com/apache/druid/pull/12096#discussion_r779950898
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/ZkWorker.java
##########
@@ -63,6 +67,25 @@ 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())) {
Review comment:
agreed, good suggestion
--
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