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/01/19 22:32:04 UTC
[GitHub] [druid] capistrant opened a new pull request #9224: Create new
dynamic config to pause coordinator helpers when needed
capistrant opened a new pull request #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224
<!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
<!-- Replace XXXX with the id of the issue fixed in this PR. Remove this section if there is no corresponding issue. Don't reference the issue in the title of this pull-request. -->
<!-- If you are a committer, follow the PR action item checklist for committers:
https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
### Description
<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
<!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
Add a new dynamic configuration for the Coordinator service. This configuration allows the coordinator to be paused. By paused, I mean that it doesn't execute Coordinator Helpers every coordination interval. Instead, if it is paused it simply logs an info log saying that it is paused and won't run the helpers.
What prompted this at my organization is the need to execute maintenance with downtime on our HDFS name nodes that are running the HDFS cluster used for deep store. We don't want the coordinator executing any of its jobs during this maintenance, but we also don't want to have to take the coordinator hard down because during the maintenance we need to restart our historical nodes with new config files to talk to hdfs and historical nodes talk to the coordinator at startup.
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
<hr>
This PR has:
- [X] been self-reviewed.
- [X] added documentation for new or modified features or behaviors.
- [X] added unit tests or modified existing tests to cover new code paths.
- [ ] added integration tests `I'm looking into writing an integration test for this`
- [X] been tested in a test Druid cluster.
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist above are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
<hr>
##### Key changed/added classes in this PR
* `DruidCoordinator`
* `CoordinatorDynamicConfig`
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on issue #9224: Create new dynamic
config to pause coordinator helpers when needed
Posted by GitBox <gi...@apache.org>.
capistrant commented on issue #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224#issuecomment-576853727
Upon some further inspection. I'm not really sure what a good way to test the functionality would be. The unit tests cover serde of the dynamic config. So we should catch any regression issues with the actual configuration via testing in the future. The use of the config item is super elementary. If the boolean is set, it is set, and the conditional will not return true, thus the helpers will be skipped. And whether or not the boolean is set in the config object should be covered by the config unit tests, should it not. So maybe there is not a test to be written.
Unless we wanted to write an integration test that confirms that if you run an indexing job while coordinator is paused, even though the job completes, you can poll and see the segment doesn't load for x time. and then enable coordinator and confirm segment loads. But you don't really know if you tested the feature there or if the stars aligned and the timing just happened to work out... Or maybe a test that asserts something was logged, but I don't like lines that rely on logs. very fragile.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on a change in pull request #9224:
Create new dynamic config to pause coordinator helpers when needed
Posted by GitBox <gi...@apache.org>.
capistrant commented on a change in pull request #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224#discussion_r375366919
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -678,9 +678,20 @@ public void run()
.withEmitter(emitter)
.withBalancerStrategy(balancerStrategy)
.build();
+
+ boolean coordinationPaused = getDynamicConfigs().getPauseCoordination();
+ if (coordinationPaused && coordLeaderSelector.isLeader()) {
Review comment:
I agree that this can be flipped to a debug log. It was nice to have it as info for our initial testing to make sure everything was squared away. I will swap to debug and add to the conditional check to cover the case you mentioned. Now that we know everything works, admins will just need to reference the dynamic config instead of logs for clarity on the state of this (which seems like better admin practice anyways).
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson merged pull request #9224: Create new dynamic
config to pause coordinator helpers when needed
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #9224:
Create new dynamic config to pause coordinator helpers when needed
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224#discussion_r375119949
##########
File path: docs/configuration/index.md
##########
@@ -1839,4 +1842,4 @@ Supported query contexts:
|`druid.router.http.readTimeout`|The timeout for data reads from Broker processes.|`PT15M`|
|`druid.router.http.numMaxThreads`|Maximum number of worker threads to handle HTTP requests and responses|`max(10, ((number of cores * 17) / 16 + 2) + 30)`|
|`druid.router.http.numRequestsQueued`|Maximum number of requests that may be queued to a destination|`1024`|
-|`druid.router.http.requestBuffersize`|Size of the content buffer for receiving requests. These buffers are only used for active connections that have requests with bodies that will not fit within the header buffer|`8 * 1024`|
\ No newline at end of file
+|`druid.router.http.requestBuffersize`|Size of the content buffer for receiving requests. These buffers are only used for active connections that have requests with bodies that will not fit within the header buffer|`8 * 1024`|
Review comment:
What has changed in this line?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #9224:
Create new dynamic config to pause coordinator helpers when needed
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224#discussion_r375117357
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -678,9 +678,20 @@ public void run()
.withEmitter(emitter)
.withBalancerStrategy(balancerStrategy)
.build();
+
+ boolean coordinationPaused = getDynamicConfigs().getPauseCoordination();
+ if (coordinationPaused && coordLeaderSelector.isLeader()) {
Review comment:
I think this `if` clause should check `startingLeaderCounter == coordLeaderSelector.localTerm()` as well as in the below `if` clause. Or maybe, this log can be just printed when the dynamic configuration is updated. Also we are trying to reduce the amount of logs. Do you think it's worth to be the info level? Or can it be the debug level?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on issue #9224: Create new dynamic
config to pause coordinator helpers when needed
Posted by GitBox <gi...@apache.org>.
capistrant commented on issue #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224#issuecomment-580032152
I was worried that the Travis failure on the realtime integration test was legit, but I was able to run the whole integration suite on my local again just now.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on a change in pull request #9224:
Create new dynamic config to pause coordinator helpers when needed
Posted by GitBox <gi...@apache.org>.
capistrant commented on a change in pull request #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224#discussion_r375395059
##########
File path: docs/configuration/index.md
##########
@@ -1839,4 +1842,4 @@ Supported query contexts:
|`druid.router.http.readTimeout`|The timeout for data reads from Broker processes.|`PT15M`|
|`druid.router.http.numMaxThreads`|Maximum number of worker threads to handle HTTP requests and responses|`max(10, ((number of cores * 17) / 16 + 2) + 30)`|
|`druid.router.http.numRequestsQueued`|Maximum number of requests that may be queued to a destination|`1024`|
-|`druid.router.http.requestBuffersize`|Size of the content buffer for receiving requests. These buffers are only used for active connections that have requests with bodies that will not fit within the header buffer|`8 * 1024`|
\ No newline at end of file
+|`druid.router.http.requestBuffersize`|Size of the content buffer for receiving requests. These buffers are only used for active connections that have requests with bodies that will not fit within the header buffer|`8 * 1024`|
Review comment:
I actually have no idea what is up here. The lines are identical so it has to be something to do with a hidden character... The rich diff shows not change for this line so whatever it is, it isn't visible. Still trying to figure it out.
**Edit: Fixed this after realizing it was an intellij thing**
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #9224:
Create new dynamic config to pause coordinator helpers when needed
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224#discussion_r375120533
##########
File path: docs/configuration/index.md
##########
@@ -763,6 +764,8 @@ Issuing a GET request at the same URL will return the spec that is currently in
|`maxSegmentsInNodeLoadingQueue`|The maximum number of segments that could be queued for loading to any given server. This parameter could be used to speed up segments loading process, especially if there are "slow" nodes in the cluster (with low loading speed) or if too much segments scheduled to be replicated to some particular node (faster loading could be preferred to better segments distribution). Desired value depends on segments loading speed, acceptable replication time and number of nodes. Value 1000 could be a start point for a rather big cluster. Default value is 0 (loading queue is unbounded) |0|
|`decommissioningNodes`| List of historical servers to 'decommission'. Coordinator will not assign new segments to 'decommissioning' servers, and segments will be moved away from them to be placed on non-decommissioning servers at the maximum rate specified by `decommissioningMaxPercentOfMaxSegmentsToMove`.|none|
|`decommissioningMaxPercentOfMaxSegmentsToMove`| The maximum number of segments that may be moved away from 'decommissioning' servers to non-decommissioning (that is, active) servers during one Coordinator run. This value is relative to the total maximum segment movements allowed during one run which is determined by `maxSegmentsToMove`. If `decommissioningMaxPercentOfMaxSegmentsToMove` is 0, segments will neither be moved from _or to_ 'decommissioning' servers, effectively putting them in a sort of "maintenance" mode that will not participate in balancing or assignment by load rules. Decommissioning can also become stalled if there are no available active servers to place the segments. By leveraging the maximum percent of decommissioning segment movements, an operator can prevent active servers from overload by prioritizing balancing, or decrease decommissioning time instead. The value should be between 0 and 100.|70|
+|`pauseCoordinator`| Boolean flag for whether or not the coordinator should execute its various jobs of coordinating the cluster. Setting this to true essentially pauses all coordination work while allowing the API to remain up. An example of when an admin may want to pause coordination would be if they are doing deep store maintenance on HDFS Name Nodes with downtime and don't want the coordinator to be directing Historical Nodes to hit the Name Node with API requests until maintenance is done and the deep store is declared healthy for use again. |false|
Review comment:
I think it's worth to document the exact list of jobs that will stop when this is set to true. Can you add it 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #9224:
Create new dynamic config to pause coordinator helpers when needed
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224#discussion_r375121011
##########
File path: integration-tests/src/test/java/org/apache/druid/tests/indexer/ITTestCoordinatorPausedTest.java
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.tests.indexer;
+
+import com.google.inject.Inject;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.coordinator.CoordinatorDynamicConfig;
+import org.apache.druid.testing.clients.CoordinatorResourceTestClient;
+import org.apache.druid.testing.guice.DruidTestModuleFactory;
+import org.apache.druid.testing.utils.ITRetryUtil;
+import org.testng.annotations.Guice;
+import org.testng.annotations.Test;
+
+import java.io.Closeable;
+import java.util.concurrent.TimeUnit;
+
+@Guice(moduleFactory = DruidTestModuleFactory.class)
+public class ITTestCoordinatorPausedTest extends AbstractITBatchIndexTest
Review comment:
Thank you for adding this test!
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on issue #9224: Create new dynamic
config to pause coordinator helpers when needed
Posted by GitBox <gi...@apache.org>.
capistrant commented on issue #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224#issuecomment-581982322
@jihoonson From your initial look, did you have any reservations with implementation details? My team has been testing this internally on non-prod environments for a bit now trying to smoke out any unexpected issues. We still haven't had any undesirable side-effects on our side as of yet.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
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 #9224:
Create new dynamic config to pause coordinator helpers when needed
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224#discussion_r374821486
##########
File path: docs/configuration/index.md
##########
@@ -763,6 +764,8 @@ Issuing a GET request at the same URL will return the spec that is currently in
|`maxSegmentsInNodeLoadingQueue`|The maximum number of segments that could be queued for loading to any given server. This parameter could be used to speed up segments loading process, especially if there are "slow" nodes in the cluster (with low loading speed) or if too much segments scheduled to be replicated to some particular node (faster loading could be preferred to better segments distribution). Desired value depends on segments loading speed, acceptable replication time and number of nodes. Value 1000 could be a start point for a rather big cluster. Default value is 0 (loading queue is unbounded) |0|
|`decommissioningNodes`| List of historical servers to 'decommission'. Coordinator will not assign new segments to 'decommissioning' servers, and segments will be moved away from them to be placed on non-decommissioning servers at the maximum rate specified by `decommissioningMaxPercentOfMaxSegmentsToMove`.|none|
|`decommissioningMaxPercentOfMaxSegmentsToMove`| The maximum number of segments that may be moved away from 'decommissioning' servers to non-decommissioning (that is, active) servers during one Coordinator run. This value is relative to the total maximum segment movements allowed during one run which is determined by `maxSegmentsToMove`. If `decommissioningMaxPercentOfMaxSegmentsToMove` is 0, segments will neither be moved from _or to_ 'decommissioning' servers, effectively putting them in a sort of "maintenance" mode that will not participate in balancing or assignment by load rules. Decommissioning can also become stalled if there are no available active servers to place the segments. By leveraging the maximum percent of decommissioning segment movements, an operator can prevent active servers from overload by prioritizing balancing, or decrease decommissioning time instead. The value should be between 0 and 100.|70|
+|`pauseCoordinator`| Boolean flag for whether or not the coordinator should execute its various jobs of coordinating the cluster. Setting this to true essentially pauses all coordination work while allowing the API to remain up. An example of when an admin may want to pause coordination would be if they are doing deep store maintenance on HDFS Name Nodes with downtime and don't want the coordinator to be directing Historical Nodes to hit the Name Node with API requests until maintenance is done and the deep store is declared healthy for use again. |false|
Review comment:
“deep store” -> “deep storage”?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on issue #9224: Create new dynamic
config to pause coordinator helpers when needed
Posted by GitBox <gi...@apache.org>.
jihoonson commented on issue #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224#issuecomment-579560608
@capistrant sorry for the delayed review. I think this feature is useful. Would you please fix the conflicts?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on issue #9224: Create new dynamic
config to pause coordinator helpers when needed
Posted by GitBox <gi...@apache.org>.
capistrant commented on issue #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224#issuecomment-576054971
The unit test changes cover serde for the DynamicConfig. It feels like some type of integration test for this may be nice too. But I wanted to get the PR opened ASAP to get feedback on the idea and implementation.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on a change in pull request #9224:
Create new dynamic config to pause coordinator helpers when needed
Posted by GitBox <gi...@apache.org>.
capistrant commented on a change in pull request #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224#discussion_r375365514
##########
File path: docs/configuration/index.md
##########
@@ -1839,4 +1842,4 @@ Supported query contexts:
|`druid.router.http.readTimeout`|The timeout for data reads from Broker processes.|`PT15M`|
|`druid.router.http.numMaxThreads`|Maximum number of worker threads to handle HTTP requests and responses|`max(10, ((number of cores * 17) / 16 + 2) + 30)`|
|`druid.router.http.numRequestsQueued`|Maximum number of requests that may be queued to a destination|`1024`|
-|`druid.router.http.requestBuffersize`|Size of the content buffer for receiving requests. These buffers are only used for active connections that have requests with bodies that will not fit within the header buffer|`8 * 1024`|
\ No newline at end of file
+|`druid.router.http.requestBuffersize`|Size of the content buffer for receiving requests. These buffers are only used for active connections that have requests with bodies that will not fit within the header buffer|`8 * 1024`|
Review comment:
I think my editor deleted the newline at the end of the file. Will 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant edited a comment on issue #9224: Create new
dynamic config to pause coordinator helpers when needed
Posted by GitBox <gi...@apache.org>.
capistrant edited a comment on issue #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224#issuecomment-576853727
**Edit:** I ended up doing my best to add an integration test to flex this. Had to change some method signatures to get the helper methods that execute tasks to give up before segments were loaded, etc. But all refactoring of those helpers is compatible with existing tests (verified on local). Interested to see what others think of this test. **end edit.**
Upon some further inspection. I'm not really sure what a good way to test the functionality would be. The unit tests cover serde of the dynamic config. So we should catch any regression issues with the actual configuration via testing in the future. The use of the config item is super elementary. If the boolean is set, it is set, and the conditional will not return true, thus the helpers will be skipped. And whether or not the boolean is set in the config object should be covered by the config unit tests, should it not. So maybe there is not a test to be written.
Unless we wanted to write an integration test that confirms that if you run an indexing job while coordinator is paused, even though the job completes, you can poll and see the segment doesn't load for x time. and then enable coordinator and confirm segment loads. But you don't really know if you tested the feature there or if the stars aligned and the timing just happened to work out... Or maybe a test that asserts something was logged, but I don't like lines that rely on logs. very fragile.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on a change in pull request #9224:
Create new dynamic config to pause coordinator helpers when needed
Posted by GitBox <gi...@apache.org>.
capistrant commented on a change in pull request #9224: Create new dynamic config to pause coordinator helpers when needed
URL: https://github.com/apache/druid/pull/9224#discussion_r375395059
##########
File path: docs/configuration/index.md
##########
@@ -1839,4 +1842,4 @@ Supported query contexts:
|`druid.router.http.readTimeout`|The timeout for data reads from Broker processes.|`PT15M`|
|`druid.router.http.numMaxThreads`|Maximum number of worker threads to handle HTTP requests and responses|`max(10, ((number of cores * 17) / 16 + 2) + 30)`|
|`druid.router.http.numRequestsQueued`|Maximum number of requests that may be queued to a destination|`1024`|
-|`druid.router.http.requestBuffersize`|Size of the content buffer for receiving requests. These buffers are only used for active connections that have requests with bodies that will not fit within the header buffer|`8 * 1024`|
\ No newline at end of file
+|`druid.router.http.requestBuffersize`|Size of the content buffer for receiving requests. These buffers are only used for active connections that have requests with bodies that will not fit within the header buffer|`8 * 1024`|
Review comment:
I actually have no idea what is up here. The lines are identical so it has to be something to do with a hidden character... The rich diff shows not change for this line so whatever it is, it isn't visible. Still trying to figure it out.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org