637: Configurable correlatorId generation for RPCClient #638
Conversation
Change-Id: I17d7a214d3336ad6e5fe890857a48b457ea599b6
| @@ -79,12 +80,14 @@ | |||
| } | |||
| }; | |||
|
|
|||
| public static Supplier<String> DEFAULT_CORRELATION_ID_GENERATOR = new IncrementingCorrelationIdGenerator(""); | |||
There was a problem hiding this comment.
As-is this static instance may be used by more than one RpcClient instance. In that case Supplier#get may be invoked concurrently, as the synchronized (_continuationMap) block only ensures exclusive access for doCall invocations on the same client. This is a problem because as-is IncrementingCorrelationIdGenerator is not thread-safe.
One option would be to replace int _correlationId with an AtomicInteger (or perhaps more appropriately, an AtomicLong.)
There was a problem hiding this comment.
Good catch. This should not be static. Will remove that.
Change-Id: I3bb119f3f7debeb0d45ff81dc9154e26635f03bb
| @@ -389,14 +389,6 @@ public String getRoutingKey() { | |||
| return _continuationMap; | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
This is a breaking change. The last correlation ID must be kept in some way, likely by keeping the _correlationId property and update it in the synchronized block.
Change-Id: Ib1f90d5a25f1c5db32ff9c838cfbeb35aac5d671
| if (_correlationIdGenerator instanceof IncrementingCorrelationIdGenerator) { | ||
| return ((IncrementingCorrelationIdGenerator) _correlationIdGenerator).getCorrelationId(); | ||
| } else { | ||
| throw new UnsupportedOperationException(); | ||
| } |
There was a problem hiding this comment.
Not super fan of this code TBH. Maybe keeping a reference to the last correlation ID (a string) and trying to convert it to an integer when getCorrelationId() is called would do the job as well. This is a best-effort approach anyway, but it avoids relying on some implementation.
There was a problem hiding this comment.
This is implementation is only there for BW compatiblity, So i don't think it's a problem that it only works for the 'default' id generator. I marked the method as deprecated anyway.
|
Thanks! |
Change-Id: I17d7a214d3336ad6e5fe890857a48b457ea599b6
Proposed Changes
#637
Types of Changes
What types of changes does your code introduce to this project?
Put an
xin the boxes that applyDid remove the 'getCorrelatorId' method from RpcClient. Since i don't see a use for, did not bother adding compatiblity hacks
Checklist
Put an
xin the boxes that apply. You can also fill these out after creatingthe PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.
CONTRIBUTING.mddocumentTried it. But gave up after a few hours. I don't have time to troubleshoot all environment issues that pop up when trying to run the 'unit' tests
Further Comments