[multistage] Avoid Broker Request Id Collision#10789
[multistage] Avoid Broker Request Id Collision#10789walterddr merged 6 commits intoapache:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10789 +/- ##
============================================
- Coverage 68.51% 63.36% -5.16%
+ Complexity 6479 5256 -1223
============================================
Files 2159 2143 -16
Lines 116044 115614 -430
Branches 17569 17514 -55
============================================
- Hits 79510 73261 -6249
- Misses 30899 36989 +6090
+ Partials 5635 5364 -271
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 434 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @Nullable RequesterIdentity requesterIdentity, RequestContext requestContext) | ||
| throws Exception { | ||
| long requestId = _requestIdGenerator.incrementAndGet(); | ||
| long requestId = _multistageRequestIdGenerator.get(); |
There was a problem hiding this comment.
+1 on this. CC @Jackie-Jiang to take a look and see if there's any alternative opinion on solving this issue.
|
|
||
| public class MultiStageBrokerRequestHandler extends BaseBrokerRequestHandler { | ||
| private static final Logger LOGGER = LoggerFactory.getLogger(MultiStageBrokerRequestHandler.class); | ||
| private final Random _random = new Random(); |
There was a problem hiding this comment.
had some offline discussion, can we do something like
requestID = ((Long)((hash(brokerId) << 32)) + _requestIdGenerator.incrementAndGet()
encode brokerID and requestID with high/low bits?
this way we can still masked out the lower bit to get the exact ordering of the requests when we look at the requestID. this will be better in terms of debuggability.
There was a problem hiding this comment.
Added base-2 masking. We can also do base-10 masking to make debugging even easier. Something like:
brokerHash * 10^9 + (requestId.incrementAndGet() % 10^9)
lmk your preference
ankitsultana
changed the title
walterddr
left a comment
walterddr
left a comment
There was a problem hiding this comment.
lgtm. also chatted offline, another alternative is to use mask ^ 10^9 + requestID
which will be simplier to decipher on logs
3 participants
cc: @walterddr
request id collisions across brokers can cause weird query failures (we are seeing this in our production).
Later we should switch to string based request-id. For now going with RNG to avoid having to deal with broker response changes and b/w incompatibility issues.