jineshparakh commented on code in PR #19278:
URL: https://github.com/apache/pinot/pull/19278#discussion_r3863218440
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/http/HttpClient.java:
##########
@@ -278,8 +278,16 @@ public SimpleHttpResponse sendRequest(ClassicHttpRequest
request)
public SimpleHttpResponse sendRequest(ClassicHttpRequest request, long
socketTimeoutMs)
throws IOException {
+ // Besides the per-request response (socket) timeout, explicitly bound the
connection-request
+ // (pool checkout) wait instead of silently inheriting the Apache
HttpClient default, so a
+ // saturated connection pool cannot block a replace/upload request
unboundedly. The TCP connect
+ // timeout is applied at the connection-manager level and is tunable via
+ // http.client.connectionTimeoutMs (see HttpClientConfig).
RequestConfig requestConfig =
-
RequestConfig.custom().setResponseTimeout(Timeout.ofMilliseconds(socketTimeoutMs)).build();
+ RequestConfig.custom()
+ .setResponseTimeout(Timeout.ofMilliseconds(socketTimeoutMs))
+
.setConnectionRequestTimeout(Timeout.ofMilliseconds(DEFAULT_CONNECTION_REQUEST_TIMEOUT_MS))
Review Comment:
Minor: I think this is a no-op on the pinned httpclient5, and the comment
above reads the other way.
`RequestConfig.custom().build()` already defaults `connectionRequestTimeout`
to 3 minutes, and `DEFAULT_CONNECTION_REQUEST_TIMEOUT_MS` is `180 * 1000` with
the in-file note *"match RequestConfig.DEFAULT_CONNECTION_REQUEST_TIMEOUT for
backwards compatibility"*. Probed against the version `pinot-common` actually
resolves (httpclient5 5.6.4):
```
before (master: setResponseTimeout only) connectionRequestTimeout = 3 MINUTES
after (this PR) connectionRequestTimeout = 180000
MILLISECONDS
equal = true
```
So the pool checkout was already bounded at 3 min and the value is unchanged
— the comment's *"instead of silently inheriting the Apache HttpClient default,
so a saturated connection pool cannot block a replace/upload request
unboundedly"* isn't quite what happens. Two options:
1. Reword to the rationale that does hold — pinning the value so a future
library default change can't silently move it.
2. If per-deployment tuning is the goal, add the real knob:
`HttpClientConfig` has `maxConnTotal`, `maxConnPerRoute`,
`disableDefaultUserAgent`, `connectionTimeoutMs` and no connection-request
field, so this timeout genuinely isn't configurable today.
##########
pinot-core/src/main/java/org/apache/pinot/core/common/MinionConstants.java:
##########
@@ -53,6 +53,10 @@ private MinionConstants() {
public static final String INITIAL_RETRY_DELAY_MS_KEY =
"initialRetryDelayMs";
public static final String RETRY_SCALE_FACTOR_KEY = "retryScaleFactor";
+ /// Per-task socket timeout (ms) for the minion -> controller segment upload
request. Defaults to
+ ///
[org.apache.pinot.common.utils.http.HttpClient#DEFAULT_SOCKET_TIMEOUT_MS] when
unset.
+ public static final String SEGMENT_UPLOAD_REQUEST_TIMEOUT_MS_KEY =
"segmentUploadRequestTimeoutMs";
Review Comment:
Minor: this knob only takes effect on **TAR** push.
`BaseMultipleSegmentsConversionExecutor.pushSegment` routes
`push.mode=METADATA` through `SegmentPushUtils.sendSegmentUriAndMetadata`,
which derives its socket timeout from `getSocketTimeoutMs(spec)` →
`spec.getTlsSpec().getReadTimeout()`, falling back to
`HttpClient.DEFAULT_SOCKET_TIMEOUT_MS`. No minion task ever sets a `TlsSpec`
(grep for `TlsSpec` across `pinot-plugins/pinot-minion-tasks` is empty;
`BaseTaskExecutor.generateSegmentGenerationJobSpec` sets table / cluster / auth
only), so on metadata push the upload timeout is hard-pinned at 10 min with no
way to change it.
TAR is the default push mode (`getSegmentPushType` defaults to
`SegmentPushType.TAR`), so the knob does cover the default path — it's just
silently inert once someone sets `push.mode=METADATA`. Either plumb the value
into the job spec, or scope this doc comment to "TAR push".
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/PinotHelixResourceManager.java:
##########
@@ -247,6 +249,11 @@ private enum LineageUpdateType {
@Nullable
private final ControllerConf _controllerConf;
private final AuthProvider _serverAdminAuthProvider;
+ // endReplaceSegments IdealState -> ExternalView convergence knobs, resolved
once from controller
+ // config (or the static defaults when no config is supplied).
+ private final long _segmentReplaceExternalViewMaxWaitMs;
+ private final long _segmentReplaceExternalViewCheckIntervalMs;
+ private final RetryPolicy _endReplaceSegmentsRetryPolicy;
Review Comment:
Three small cleanups, all optional:
1. **Dependency direction.** `ControllerConf` now imports
`PinotHelixResourceManager` for two of the three defaults, while the third
(`DEFAULT_SEGMENT_REPLACE_MAX_RETRY_ATTEMPTS`) lives in `ControllerConf`
itself. Config → resource manager is inverted, and it makes `o.a.p.controller`
↔ `o.a.p.controller.helix.core` a package cycle. Moving all three defaults into
`ControllerConf` and having this class read them from there would be more
consistent (keep the `EXTERNAL_VIEW_*` constants as aliases —
`OfflineClusterIntegrationTest` imports them statically).
2. **Caching.** The two longs in fields plus the constructor `if/else` is a
little more than needed; this class already handles the nullable conf at the
use site, e.g. `_controllerConf == null ||
_controllerConf.isLineageExclusiveDeleteEnabled()`. Reading at the use site
also leaves room for a cluster-config-backed override later, whereas resolving
in the constructor pins the values for the controller's lifetime (a restart to
retune a knob whose main use is incident-time tuning).
3. **Test depth.** The new tests in `ControllerConfTest` / `MinionConfTest`
assert `getProperty` round-trips, so they'd still pass if this class ignored
the config entirely — which is the regression that would actually hurt. A test
that the resolved values reach the wait loop and the retry policy would cover
it; marking `waitForSegmentsBecomeOnline` `@VisibleForTesting` package-private
makes that cheap. (I confirmed the wiring works today by reflecting on a
constructed `PinotHelixResourceManager`: configured 45 min / 500 ms / 3 lands
in these fields, unset gives 10 min / 1 s / 5.)
##########
pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java:
##########
@@ -459,6 +460,18 @@ public static long getRandomInitialDelayInSeconds() {
public static final String CONFIG_OF_MAX_RELOAD_SEGMENT_JOBS_IN_ZK =
"controller.reload.segment.maxJobsInZK";
public static final String CONFIG_OF_MAX_FORCE_COMMIT_JOBS_IN_ZK =
"controller.force.commit.maxJobsInZK";
+ // Knobs governing how long endReplaceSegments blocks while waiting for the
new segments to become
+ // ONLINE in the ExternalView (IdealState -> ExternalView convergence). The
per-attempt wait is
+ // retried up to the configured number of attempts, so the worst-case block
is
+ // maxWaitMs * maxRetryAttempts. Defaults preserve the historical hard-coded
values.
Review Comment:
Minor (docs): now that both ends are tunable, it would help to state the
intended ordering somewhere, because the defaults are currently inverted.
| timeout | default |
|---|---|
| Helix per-task (`JobConfig.DEFAULT_TIMEOUT_PER_TASK`, applied via
`setTimeoutPerTask`) | 1 h |
| controller worst case (`maxRetryAttempts × externalViewMaxWaitMs` +
backoff) | ~50 min |
| minion `pinot.minion.endReplaceSegments.timeoutMs` | 10 min |
So out of the box the minion abandons the request while the controller keeps
a request thread blocked for up to ~40 min more, and `endSegmentReplace` has no
client-side retry — the task fails and the next run issues a fresh
`startSegmentReplace(forceCleanup=true)`. Raising `externalViewMaxWaitMs`
widens that window.
A line here and on the minion key spelling out `helix task timeout > minion
endReplaceSegments.timeoutMs > maxRetryAttempts × externalViewMaxWaitMs`, or
just logging the resolved values at controller startup, would make these safe
to turn. Worth calling out explicitly that the exposed unit is **per-attempt**,
not a total budget (the comment does say `maxWaitMs * maxRetryAttempts`, which
is what I verified: with `attempts=3` the callable — and therefore the EV wait
— runs 3 times).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
