KAFKA-6299. Fix AdminClient error handling when metadata changes by cmccabe · Pull Request #4295 · apache/kafka

cmccabe · 2017-12-06T00:42:07Z

AdminClient should only call Metadata#requestUpdate when needed.

When AdminClient gets a NOT_CONTROLLER error, it should refresh its metadata and retry the request, rather than making the end-user deal with NotControllerException.

Move AdminClient's metadata management outside of NetworkClient and into AdminMetadataManager. This will make it easier to do more sophisticated metadata management in the future, such as implementing a NodeProvider which fetches the leaders for topics.

Rather than manipulating newCalls directly, the AdminClient service thread now drains it directly into pendingCalls. This minimizes the amount of locking we have to do, since pendingCalls is only accessed from the service thread.

tedyu · 2017-12-06T01:10:22Z

Should this if block be placed above the if block on line 729 ?

Hmm-- I don't think so. We don't want to return the exception of the stale metadata, if new metadata has been requested.

tedyu · 2017-12-06T01:11:20Z

Probably add comment on why synchronization on callsToSend is not needed.

I will add a comment to the declaration of callsToSend, since this is used in many other places.

cmccabe · 2017-12-18T18:53:56Z

Test failure was org.apache.kafka.common.security.authenticator.ClientAuthenticationFailureTest.testAdminClientWithInvalidCredentials, which is not related.

hachikuji · 2018-04-12T16:13:33Z

@cmccabe The test failure does seem caused by this patch. I reproduced locally on this branch. Can you check again?

hachikuji

Thanks for the patch. Left a few comments/questions.

hachikuji · 2018-04-12T19:03:40Z

This is kind of a weird contract. Maybe the name should be checkMetadataFetchError or something like that?

checkMetadataError might be a better name

hachikuji · 2018-04-12T20:51:17Z

Is this necessary? It seems like the only way it's possible for the if condition to be true is if we already updated lastSeenMetadata below.

The point of this if statement is that we shouldn't keep calling Metadata#requestUpdate over and over. We should call it once, wait for an update, and then call it again after that if needed.

I was not questioning the need of the if, but the reassignment to lastSeenMetadataVersion, which seemed redundant given how it is updated below. I may have answered my own question however. The problem is that the version in Metadata is updated in two additional cases: 1) on initial bootstrapping, and 2) after a periodic refresh. We handle the first case I think because of this reassignment and the fact that the two fields are initialized to 0.

I'm not sure about the second case, however. Say, for example, that lowestValidMetadataVersion and lastSeenMetadataVersion are both 5 and no update has been requested. After the metadata max age expires, we'll automatically trigger an update and bump lastSeenMetadataVersion to 6. Now if we request a metadata update, nothing will happen because we'll never reach equality again. We may be able to fix it by changing to an inequality, but I'm not sure. Some testing would be helpful.

hachikuji · 2018-04-12T21:01:07Z

nit: not that big of a deal, but since we do the same thing in several places, maybe we could add a requestMetadataUpdate method to the runnable.

hachikuji · 2018-04-12T21:09:15Z

Is the NOT_CONTROLLER error possible for Metadata requests? Also, are there any other errors we care about at this level (e.g. NOT_LEADER errors)?

No, it's not. Good catch.

"Not leader for partition" is something we care about for certain calls, but handling it will be harder. I want to do that in a follow-on change.

hachikuji · 2018-04-12T21:15:04Z

I guess I would have expected that these transient errors would get retried internally. Is the intent to do this separately?

Good question. Unfortunately, they can't be retried internally because we have no generic top level error code. So if createTopics fails, you get back a response that has {foo : NOT_CONTROLLER, bar: NOT_CONTROLLER, baz: NOT_CONTROLLER, etc.} You can't parse this without knowing the subtype, which means it has to be done here.

To clarify my question, does the user request fail because of a transient NOT_CONTROLLER error or do we retry the request? I had expected we would retry, but perhaps we are leaving that for future work?

hachikuji · 2018-04-12T21:20:35Z

Have you verified that this works for all uses of version at the moment? It seems like a minor change in behavior since we use failedUpdate in DefaultMetadataUpdater if there are no nodes in the response. Currently this would cause uses such as ConsumerNetworkClient.awaitMetadataUpdate() to retry before returning.

I don't think retrying is what we want, though. If you get an authentication exception from any broker, your auth is bad and you should fail. It's not a retryable exception, conceptually or in terms of code

The case I am referring to is in DefaultMetadataupdater.handleCompletedMetadataResponse, which is different from the authentication failure path. We have a check to ensure that the metadata response contains at least one broker. With this change, we will update the metadata version even in the case that it is empty, which will cause methods like ConsumerNetworkClient.awaitMetadataUpdate() to return earlier than they currently do.

Hmm. Maybe I'm misinterpreting, but are you suggesting that ConsumerNetworkClient#awaitMetadataUpdate() should wait forever (or until the timeout hits) when there is an authentication error fetching the metadata? That doesn't seem right.

I am definitely not suggesting that. I am not talking about authentication failures at all. I am referring to the call to Metadata.failedUpdate that is in DefaultMetadataupdater.handleCompletedMetadataResponse. With this change, the version will be incremented following an "empty" metadata update. This will cause us to exit the loop in ConsumerNetworkClient#awaitMetadataUpdate() even though we have not received a valid update.

My point more generally is that we are changing the meaning of the version field inside Metadata. It seems we were intentionally using this before to indicate when we had received a valid new version of the metadata. But now we bump it even when there's a failure which means we can no longer use it to tell when we've seen a successful update. As far as I can tell, the impact may be minor, but we should consider all of the usages to be sure of it.

hachikuji · 2018-04-12T21:24:19Z

Seems this was not really necessary since we just needed a sentinel value? Note that common/errors is a public package.

I can use NetworkException instead (which is a subclass of InvalidMetadataException)

hachikuji · 2018-04-17T18:14:31Z

+            env.kafkaClient().setNode(env.cluster().nodeById(0));
+            env.kafkaClient().prepareResponse(new CreateTopicsResponse(Collections.singletonMap("myTopic", new ApiError(Errors.NONE, ""))));
+            KafkaFuture<Void> future = env.adminClient().createTopics(
+                Collections.singleton(new NewTopic("myTopic", Collections.singletonMap(Integer.valueOf(0), asList(new Integer[]{0, 1, 2})))),


nit: this can be simplified Collections.singletonMap(0, asList(0, 1, 2)

A few more of these in this file.

hachikuji · 2018-04-17T18:42:14Z

I was not questioning the need of the if, but the reassignment to lastSeenMetadataVersion, which seemed redundant given how it is updated below. I may have answered my own question however. The problem is that the version in Metadata is updated in two additional cases: 1) on initial bootstrapping, and 2) after a periodic refresh. We handle the first case I think because of this reassignment and the fact that the two fields are initialized to 0.

I'm not sure about the second case, however. Say, for example, that lowestValidMetadataVersion and lastSeenMetadataVersion are both 5 and no update has been requested. After the metadata max age expires, we'll automatically trigger an update and bump lastSeenMetadataVersion to 6. Now if we request a metadata update, nothing will happen because we'll never reach equality again. We may be able to fix it by changing to an inequality, but I'm not sure. Some testing would be helpful.

cmccabe · 2018-04-23T18:30:51Z

Jenkins is flaking again due to out of memory errors launching git.

18:28:10 Caused by: java.lang.OutOfMemoryError: unable to create new native thread
18:28:10 	at java.lang.Thread.start0(Native Method)
18:28:10 	at java.lang.Thread.start(Thread.java:717)
18:28:10 	at hudson.Proc$LocalProc.<init>(Proc.java:269)
18:28:10 	at hudson.Proc$LocalProc.<init>(Proc.java:218)
18:28:10 	at hudson.Launcher$LocalLauncher.launch(Launcher.java:930)
18:28:10 	at hudson.Launcher$ProcStarter.start(Launcher.java:450)
18:28:10 	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1992)
18:28:10 	... 15 more
18:28:10 ERROR: Error cloning remote repo 'origin'
18:28:10 Retrying after 10 seconds

cmccabe · 2018-04-23T18:30:57Z

retest this please

hachikuji

Thanks, the approach using the custom metadata updater seems promising. Left some comments/questions.

hachikuji · 2018-05-03T18:48:19Z

This is a public package, which conventionally has implied that all of the classes in it are public as well. Should we have an internals package for stuff like this? This is the pattern we use for the consumer and producer.

OK, I'll create an internals package, to follow the convention.

hachikuji · 2018-05-03T21:40:35Z

nit: comments like this seem like overkill

fair enough

hachikuji · 2018-05-03T21:47:52Z

Would it make sense to extend ManualMetadataUpdater. It already has most of the no-op functionality we want.

I guess my thought process here is that the broker is using ManualMetadataUpdater, and I don't want changes to ManualMetadataUpdater to change AdminClient. Since the (lack of?) functionality here is minimal, seems better just to create another class.

hachikuji · 2018-05-03T22:58:13Z

nit: use the other constructor?

hachikuji · 2018-05-03T23:16:14Z

It's a little unclear if we need this. Below when we see a disconnect, we check for authentication errors explicitly. Do we need anything else? It would be a little clearer if we only have one path for surfacing authentication errors.

When NetworkClient gets a request, it sometimes has to make additional internal requests to fulfill it. For example, if NC gets asked to make an AlterConfigsRequest, it may first have to make an API versions request. However, if this API versions request fails, there is nothing which ties the failure back to the AlterConfigsRequest. Since it's an "internal" request, the failure disappears without a trace and never makes its way into the list of responses.

I would argue that this is a bug in NetworkClient. We are currently hacking around it by things like having the event loop manually iterate through each node in ClusterConnectionStates to see whether any of them ended up in ConnectionState.AUTHENTICATION_FAILED. The metadata updater is another hack which gets triggered even when the Response gets dropped, in NetworkClient#processDisconnection.

If we want to be a little braver, we could say that when handling an internal APIVersionRequest disconnect, we could also send a disconnection to the next non-internal request queued for that node.

Really the whole concept of an internal request is evil. We should just have a wrapper class around NetworkClient that translates one stream of requests into another. That would help us keep this straight. But that is too big to do right now.

hachikuji · 2018-05-03T23:32:21Z

Is this not a concurrent modification?

Technically, the iterator removes the entry from pendingAuthenticationErrors via Iterator#remove, and then the call to pendingAuthenticationErrors#remove has no effect.

hachikuji · 2018-05-04T01:24:34Z

The name timeoutMs seems misleading. It's really the blackout period following the authentication error?

Good point. I will change it to blackoutMs.

hachikuji · 2018-05-04T15:39:54Z

What is the expected behavior if the user ignores the auth error and continues to use the AdminClient?

It will continue to throw AuthenticationException.

After enough time, another metadata request may be made which may succeed, which would allow future requests to go through. But we don't spam metadata requests or anything-- if the auth exception is cleared, it will be because of a timeout.

hachikuji · 2018-05-04T22:22:41Z

nit: not saving much..

hachikuji · 2018-05-04T22:32:09Z

Should the poll timeout take into account the time to the next metadata refresh? What happens if we are in the middle of the metadata backoff when a call is made?

Yes, it should. Will fix.

cmccabe · 2018-05-09T00:50:03Z

retest this please

hachikuji · 2018-05-09T16:57:10Z

Is the synchronization needed here since pendingCalls is only accessed by this thread?

It's not needed. Good catch.

hachikuji · 2018-05-09T17:05:28Z

We don't need the second {} since the logging treats the exception specially.

When AdminClient gets a NOT_CONTROLLER error, it should refresh its metadata and retry the request, rather than making the end-user deal with NotControllerException. Move AdminClient's metadata management outside of NetworkClient and into AdminMetadataManager. This will make it easier to do more sophisticated metadata management in the future, such as implementing a NodeProvider which fetches the leaders for topics. Rather than manipulating newCalls directly, the AdminClient service thread now drains it directly into pendingCalls. This minimizes the amount of locking we have to do, since pendingCalls is only accessed from the service thread.

hachikuji · 2018-05-09T17:18:00Z

+
+    public boolean isReady() {
+        if (authException != null) {
+            log.trace("Metadata is ready: got authentication exception.");


Should this be "Metadata is not ready"?

hachikuji

Thanks for the patch, LGTM. Will merge after the builds complete.

hachikuji · 2018-05-09T17:40:06Z

retest this please

…-record-version * apache-github/trunk: KAFKA-6894: Improve err msg when connecting processor with global store (apache#5000) KAFKA-6893; Create processors before starting acceptor in SocketServer (apache#4999) MINOR: Fix typo in ConsumerRebalanceListener JavaDoc (apache#4996) MINOR: Remove deprecated valueTransformer.punctuate (apache#4993) MINOR: Update dynamic broker configuration doc for truststore update (apache#4954) KAFKA-6870 Concurrency conflicts in SampledStat (apache#4985) KAFKA-6361: Fix log divergence between leader and follower after fast leader fail over (apache#4882) KAFKA-6813: Remove deprecated APIs in KIP-182, Part II (apache#4976) KAFKA-6878 Switch the order of underlying.init and initInternal (apache#4988) KAFKA-6299; Fix AdminClient error handling when metadata changes (apache#4295) KAFKA-6878: NPE when querying global state store not in READY state (apache#4978) KAFKA 6673: Implemented missing override equals method (apache#4745) KAFKA-6834: Handle compaction with batches bigger than max.message.bytes (apache#4953)

…che#4295) When AdminClient gets a NOT_CONTROLLER error, it should refresh its metadata and retry the request, rather than making the end-user deal with NotControllerException. Move AdminClient's metadata management outside of NetworkClient and into AdminMetadataManager. This will make it easier to do more sophisticated metadata management in the future, such as implementing a NodeProvider which fetches the leaders for topics. Rather than manipulating newCalls directly, the AdminClient service thread now drains it directly into pendingCalls. This minimizes the amount of locking we have to do, since pendingCalls is only accessed from the service thread.

cmccabe mentioned this pull request Dec 6, 2017

KAFKA-5950: AdminClient should retry based on returned error codes #4167

Closed

cmccabe force-pushed the KAFKA-6299 branch from 5f54805 to 99323a9 Compare December 6, 2017 00:44

tedyu reviewed Dec 6, 2017

View reviewed changes

cmccabe force-pushed the KAFKA-6299 branch 3 times, most recently from ccadd98 to 03be633 Compare December 15, 2017 19:33

cmccabe force-pushed the KAFKA-6299 branch from 03be633 to 0547eae Compare December 18, 2017 18:59

cmccabe force-pushed the KAFKA-6299 branch from 0547eae to 8e8b9a6 Compare April 11, 2018 16:25

cmccabe force-pushed the KAFKA-6299 branch from 8e8b9a6 to 73a6840 Compare April 12, 2018 19:05

hachikuji reviewed Apr 12, 2018

View reviewed changes

cmccabe force-pushed the KAFKA-6299 branch 2 times, most recently from 10ed8a5 to 66bbf66 Compare April 13, 2018 22:14

hachikuji reviewed Apr 17, 2018

View reviewed changes

cmccabe mentioned this pull request Apr 23, 2018

KAFKA-6791: Add CoordinatorNodeProvider in KafkaAdminClient #4902

Closed

3 tasks

cmccabe force-pushed the KAFKA-6299 branch from 66bbf66 to a240fd4 Compare April 23, 2018 18:27

cmccabe force-pushed the KAFKA-6299 branch 2 times, most recently from 530d59b to 41b3f7a Compare May 2, 2018 18:26

hachikuji reviewed May 4, 2018

View reviewed changes

cmccabe force-pushed the KAFKA-6299 branch from 41b3f7a to 9641f16 Compare May 7, 2018 18:36

hachikuji reviewed May 9, 2018

View reviewed changes

cmccabe added 3 commits May 9, 2018 10:09

Fixes

8e63065

More review fixes

9e31e20

cmccabe force-pushed the KAFKA-6299 branch from 9641f16 to 9e31e20 Compare May 9, 2018 17:13

hachikuji reviewed May 9, 2018

View reviewed changes

cmccabe added 2 commits May 9, 2018 10:33

Fix TopicAdminTest to set a node to be returned from leastLoadedNodes

c106725

Change log message in AdminMetadataManager#isReady

5e52241

hachikuji approved these changes May 9, 2018

View reviewed changes

hachikuji merged commit abbd53d into apache:trunk May 9, 2018

cmccabe deleted the KAFKA-6299 branch May 20, 2019 18:55

Conversation

cmccabe commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmccabe commented Dec 18, 2017

Uh oh!

hachikuji commented Apr 12, 2018

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmccabe Apr 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmccabe commented Apr 23, 2018

Uh oh!

cmccabe commented Apr 23, 2018

Uh oh!

hachikuji left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmccabe May 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

cmccabe commented Dec 6, 2017 •

edited

Loading

cmccabe Apr 13, 2018 •

edited

Loading

cmccabe May 7, 2018 •

edited

Loading

cmccabe May 7, 2018 •

edited

Loading