KAFKA-20064: Made PartitionLeaderCache thread safe. #21335
Conversation
|
@Nikita-Shupletsov Test failures for |
|
And is this PR superseding #21298? |
fixed
this PR is from @seekskyworld. He and I sent our PRs in parallel |
AndrewJSchofield
left a comment
AndrewJSchofield
left a comment
There was a problem hiding this comment.
Thanks for the PR. Some comments from an initial review.
| // metadata. For all cached keys, they can proceed straight to the fulfillment map. | ||
| // Note that the cache is only used on the initial calls, and any errors that result | ||
| // in additional lookups use the full set of lookup keys. | ||
| retryLookup(future.uncachedLookupKeys()); |
There was a problem hiding this comment.
Can't you remove uncachedLookupKeys entirely?
There was a problem hiding this comment.
if I understand the suggestion correctly, I already have: https://github.com/apache/kafka/pull/21335/changes#diff-6414b53806c9ce1b21cd8f6966e1427789e62c927969cfd4c856660994ce0944L46
if you mean something else, please let me know, thanks!
|
|
||
| @Override | ||
| public InetAddress[] resolve(String host) throws UnknownHostException { | ||
| System.out.println("RESOLVE: " + host); |
There was a problem hiding this comment.
Please remove the println.
| adminClient = KafkaAdminClient.createInternal(new AdminClientConfig(clusterInstance.setClientSaslConfig(props), true), | ||
| null, new TestHostResolver()); | ||
|
|
||
| Field clientField = KafkaAdminClient.class.getDeclaredField("client"); |
There was a problem hiding this comment.
I would prefer TestUtils.fieldValue here.
Replaced direct reflection with TestUtils.
3 participants
cached and non-cached values as one call instead of two, deleting cached
values as one call, not one by one)
cache