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 2020/08/13 23:19:28 UTC

[GitHub] [druid] jihoonson opened a new pull request #10278: Don't log the entire task spec

jihoonson opened a new pull request #10278:
URL: https://github.com/apache/druid/pull/10278


   ### Description
   
   `HttpIndexingServiceClient` prints the entire task spec when it fails to issue a task, which could be huge especially in parallel task when lots of segments are shuffled. This could end up causing OOM error in the supervisor task. This PR fixes it by logging only task ID. I also cleaned up some wrong logging in `ParallelIndexSupervisorTask`.
   
   <hr>
   
   This PR has:
   - [x] 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.)
   - [ ] added documentation for new or modified features or behaviors.
   - [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
   - [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
   - [ ] 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.

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] abhishekagarwal87 commented on a change in pull request #10278: Don't log the entire task spec

Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #10278:
URL: https://github.com/apache/druid/pull/10278#discussion_r471011332



##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/AbstractTask.java
##########
@@ -175,23 +162,6 @@ public String toString()
            '}';
   }
 
-  /**
-   * Start helper methods
-   *
-   * @param objects objects to join
-   *
-   * @return string of joined objects
-   */
-  static String joinId(List<Object> objects)
-  {
-    return ID_JOINER.join(objects);

Review comment:
       nit: ID_JOINER field can be removed from the class. 

##########
File path: server/src/main/java/org/apache/druid/client/indexing/IndexingServiceClient.java
##########
@@ -31,11 +31,27 @@
 
 public interface IndexingServiceClient
 {
-  void killUnusedSegments(String dataSource, Interval interval);
+  default void killUnusedSegments(String dataSource, Interval interval)
+  {
+    killUnusedSegments("", dataSource, interval);

Review comment:
       naive question: can the IndexingServiceClient used outside druid code? If not, a default method may not be required. If yes, passing null instead of an empty string will be closer to past behavior. 




----------------------------------------------------------------
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.

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] jihoonson commented on a change in pull request #10278: Don't log the entire task spec

Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10278:
URL: https://github.com/apache/druid/pull/10278#discussion_r470879995



##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexSupervisorTask.java
##########
@@ -788,18 +785,7 @@ private static void publishSegments(TaskToolbox toolbox, Map<String, PushedSegme
     if (published) {
       LOG.info("Published [%d] segments", newSegments.size());
     } else {
-      LOG.info("Transaction failure while publishing segments, checking if someone else beat us to it.");

Review comment:
       Oh, I think the logging here was wrong. In parallel indexing, unlike in streaming ingestion, there is no concept of replicas which could publish the same set of segments. So, if it failed to publish segments, the task should just fail without checking if those segments were already published by another task since it cannot happen.
   
   > "Our segments really do exist, awaiting handoff."
   
   This log is also misleading since batch tasks never waits for segment handoff.




----------------------------------------------------------------
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.

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] jon-wei commented on a change in pull request #10278: Don't log the entire task spec

Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #10278:
URL: https://github.com/apache/druid/pull/10278#discussion_r471827391



##########
File path: server/src/main/java/org/apache/druid/client/indexing/ClientCompactionTaskQuery.java
##########
@@ -43,14 +42,14 @@
 
   @JsonCreator
   public ClientCompactionTaskQuery(
-      @JsonProperty("id") @Nullable String id,
+      @JsonProperty("id") String id,
       @JsonProperty("dataSource") String dataSource,
       @JsonProperty("ioConfig") ClientCompactionIOConfig ioConfig,
       @JsonProperty("tuningConfig") ClientCompactionTaskQueryTuningConfig tuningConfig,
       @JsonProperty("context") Map<String, Object> context
   )
   {
-    this.id = id == null ? IdUtils.newTaskId(TYPE, dataSource, null) : id;
+    this.id = Preconditions.checkNotNull(id, "id");

Review comment:
       For backwards compatibility, would it be better to allow a null id as before?




----------------------------------------------------------------
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.

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] jon-wei commented on a change in pull request #10278: Don't log the entire task spec

Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #10278:
URL: https://github.com/apache/druid/pull/10278#discussion_r470383854



##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexSupervisorTask.java
##########
@@ -788,18 +785,7 @@ private static void publishSegments(TaskToolbox toolbox, Map<String, PushedSegme
     if (published) {
       LOG.info("Published [%d] segments", newSegments.size());
     } else {
-      LOG.info("Transaction failure while publishing segments, checking if someone else beat us to it.");

Review comment:
       What's the reasoning for removing the more complex logging here? Is it concern from the segment set being too large?

##########
File path: server/src/main/java/org/apache/druid/client/indexing/HttpIndexingServiceClient.java
##########
@@ -67,7 +67,8 @@ public HttpIndexingServiceClient(
   @Override
   public void killUnusedSegments(String dataSource, Interval interval)
   {
-    runTask(new ClientKillUnusedSegmentsTaskQuery(dataSource, interval));
+    final ClientTaskQuery taskQuery = new ClientKillUnusedSegmentsTaskQuery(null, dataSource, interval);
+    runTask(taskQuery.getId(), taskQuery);

Review comment:
       Do we pull the ID from the `taskQuery` anywhere except here? If not, maybe we could just pass in the ID here without adding an ID field to the task query.




----------------------------------------------------------------
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.

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] lgtm-com[bot] commented on pull request #10278: Don't log the entire task spec

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #10278:
URL: https://github.com/apache/druid/pull/10278#issuecomment-673775122


   This pull request **introduces 1 alert** when merging 4525d18540cf1bb6c73429e88fc934b707c5404c into 6cca7242de629ad07478c6792b01dc1f37dc6b40 - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-16083d206585ec6d9f571fe88fb32bfcfbe8489e)
   
   **new alerts:**
   
   * 1 for Unused format argument


----------------------------------------------------------------
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.

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] jihoonson commented on a change in pull request #10278: Don't log the entire task spec

Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10278:
URL: https://github.com/apache/druid/pull/10278#discussion_r470878798



##########
File path: server/src/main/java/org/apache/druid/client/indexing/HttpIndexingServiceClient.java
##########
@@ -67,7 +67,8 @@ public HttpIndexingServiceClient(
   @Override
   public void killUnusedSegments(String dataSource, Interval interval)
   {
-    runTask(new ClientKillUnusedSegmentsTaskQuery(dataSource, interval));
+    final ClientTaskQuery taskQuery = new ClientKillUnusedSegmentsTaskQuery(null, dataSource, interval);
+    runTask(taskQuery.getId(), taskQuery);

Review comment:
       `ClientTaskQuery` is used by the coordinator when it issues kill tasks or compaction tasks. The coordinator cannot use `Task` directly because of a dependency issue, and so `ClientTaskQuery` is supposed to be equivalent to `Task`. The `id` of `taskQuery` should be same with the `id` of `task` for that reason after it is deserialized in the overlord. I think we still need an ID field for `ClientTaskQuery`.
   
   However, I like the idea of passing in the ID. When the coordinator issues a task, we can generate an ID distinguishable from those issued by users. After this PR, the ID of compaction and kill tasks will be prefixed by `coordinator-issued` if they are submitted by coordinator. `api-issued` prefix will be used when a kill task is issued by calling `DataSourcesResource.killUnusedSegmentsInInterval()`.




----------------------------------------------------------------
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.

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] jon-wei merged pull request #10278: Don't log the entire task spec

Posted by GitBox <gi...@apache.org>.
jon-wei merged pull request #10278:
URL: https://github.com/apache/druid/pull/10278


   


----------------------------------------------------------------
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.

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] jihoonson commented on a change in pull request #10278: Don't log the entire task spec

Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10278:
URL: https://github.com/apache/druid/pull/10278#discussion_r471866557



##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/AbstractTask.java
##########
@@ -175,23 +162,6 @@ public String toString()
            '}';
   }
 
-  /**
-   * Start helper methods
-   *
-   * @param objects objects to join
-   *
-   * @return string of joined objects
-   */
-  static String joinId(List<Object> objects)
-  {
-    return ID_JOINER.join(objects);

Review comment:
       Thanks, removed.

##########
File path: server/src/main/java/org/apache/druid/client/indexing/IndexingServiceClient.java
##########
@@ -31,11 +31,27 @@
 
 public interface IndexingServiceClient
 {
-  void killUnusedSegments(String dataSource, Interval interval);
+  default void killUnusedSegments(String dataSource, Interval interval)
+  {
+    killUnusedSegments("", dataSource, interval);

Review comment:
       Oh yeah, we don't need those new default methods here. I'm not sure why I added them :sweat_smile:. `IndexingServiceClient` is not a [public API](https://github.com/apache/druid/blob/master/core/src/main/java/org/apache/druid/guice/annotations/PublicApi.java). We don't support backward compatibility for those APIs.

##########
File path: server/src/main/java/org/apache/druid/client/indexing/ClientCompactionTaskQuery.java
##########
@@ -43,14 +42,14 @@
 
   @JsonCreator
   public ClientCompactionTaskQuery(
-      @JsonProperty("id") @Nullable String id,
+      @JsonProperty("id") String id,
       @JsonProperty("dataSource") String dataSource,
       @JsonProperty("ioConfig") ClientCompactionIOConfig ioConfig,
       @JsonProperty("tuningConfig") ClientCompactionTaskQueryTuningConfig tuningConfig,
       @JsonProperty("context") Map<String, Object> context
   )
   {
-    this.id = id == null ? IdUtils.newTaskId(TYPE, dataSource, null) : id;
+    this.id = Preconditions.checkNotNull(id, "id");

Review comment:
       Seems like you were seeing changes between commits. The `id` field was newly added in this PR. Before this PR, `id` was initialized when `ClientCompactionTaskQuery` is converted into `CompactionTask`. Since the `id` field in `ClientCompactionTaskQuery` was never accessed on the coordinator side, I think there should be no backward-compatibility 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.

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