xiangfu0 commented on code in PR #20021:
URL: https://github.com/apache/pinot/pull/20021#discussion_r4369188470
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java:
##########
@@ -447,11 +478,18 @@ private void processInstanceConfigChangeInternal() {
// Calculate new enabled and disabled servers
Set<String> enabledServers = new HashSet<>();
+ // Every server that still has an instance config, enabled or not. Holds
the raw ids from the records, while
+ // _enabledServerInstanceMap and _disabledServers are keyed by the
interned ids: lookups compare ids with equals(),
+ // so the two match by value; interning only gives those maps an identity
fast path.
+ Set<String> presentServers = new HashSet<>();
Review Comment:
Re-reviewed at 4b7bb7ca and the raw-vs-interned id concern from last round
is handled cleanly: the comment here spells out that `presentServers` holds raw
ZNRecord ids while the maps are keyed by interned ids, and since the lookups go
through `equals()` the two match by value. Interning stays a pure fast-path,
not a correctness dependency. Thanks for documenting it right at the
declaration.
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ServerChannels.java:
##########
@@ -146,10 +174,57 @@ public Channel sendRequest(String rawTableName,
AsyncQueryResponse asyncQueryRes
ServerRoutingInstance serverRoutingInstance, InstanceRequest
instanceRequest, long timeoutMs)
throws Exception {
byte[] requestBytes =
THREAD_LOCAL_T_SERIALIZER.get().serialize(instanceRequest);
- ServerChannel serverChannel =
_serverToChannelMap.computeIfAbsent(serverRoutingInstance, ServerChannel::new);
- // This server is now query-carrying, whoever opened the channel. See
[#hasChannel].
- serverChannel._openedByQuery = true;
- return serverChannel.sendRequest(rawTableName, asyncQueryResponse,
serverRoutingInstance, requestBytes, timeoutMs);
+ // One deadline for the whole call: a sender that races an eviction
retries on a fresh entry, but the lock waits
+ // and connects of all its attempts together stay within the caller's
timeout.
+ long deadlineMs = System.currentTimeMillis() + timeoutMs;
+ for (int attempt = 1; attempt <= MAX_SEND_ATTEMPTS; attempt++) {
Review Comment:
The single-deadline retry loop addresses the timeout-budget concern from
last round: `deadlineMs` is computed once and every attempt's lock wait and
connect draw from `max(0, deadlineMs - now)`, so a sender racing repeated
evictions still stays within the caller's original `timeoutMs` instead of
paying it per attempt. `MAX_SEND_ATTEMPTS` bounds the loop on top of that.
Looks right.
--
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]
