Conversation
| } catch (Exception e) { | ||
| if (!(e instanceof GroupAuthorizationException || e instanceof TopicAuthorizationException || e instanceof InvalidTopicException)) | ||
| if (!(e instanceof GroupAuthorizationException || e instanceof TopicAuthorizationException | ||
| || e instanceof InvalidTopicException || e instanceof UnknownServerException)) |
There was a problem hiding this comment.
Is this intended as a bug fix? the exception thrown by processBackgroundEventsOnClose will be handled by swallow
There was a problem hiding this comment.
That's true, but it will then set firstException in ShareConsumerImpl.close(Duration, boolean) and the exception will eventually be thrown by that method.
There was a problem hiding this comment.
Out of curiosity, since we are swallowing UnknownServerException in processBackgroundEventsOnClose, shouldn't we do the same in sendAcknowledgementsAndLeaveGroup for consistency? Both are steps in the close sequence.
There was a problem hiding this comment.
Yes, I was thinking that. There's no hurry to get this into 4.3 because it just fixes a minimally flaky test. I'll spend a bit more time here.
There was a problem hiding this comment.
I've decided to revert this part of the PR. Although it makes a test reliable, really the fix should be to stop the broker returning the unknown server error.
apoorvmittal10
left a comment
apoorvmittal10
left a comment
There was a problem hiding this comment.
LGTM, agree that the exception swallow is marked false when the client is instatiated correctly oterwise it will fail with original exception.
Fixes a couple of flaky tests. Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Apoorv Mittal <apoorvmittal10@gmail.com>
3 participants
Fixes a couple of flaky tests.
Reviewers: Chia-Ping Tsai chia7712@gmail.com, Apoorv Mittal
apoorvmittal10@gmail.com