Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MINOR: Ensure that selection key is cancelled on close #1368

Closed
wants to merge 1 commit into from

Conversation

rajinisivaram
Copy link
Contributor

No description provided.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@ijuma
Copy link
Contributor

ijuma commented May 11, 2016

Thanks for the PR, LGTM.

@asfgit asfgit closed this in e20eba9 May 11, 2016
asfgit pushed a commit that referenced this pull request May 11, 2016
Author: Rajini Sivaram <rajinisivaram@googlemail.com>

Reviewers: Ismael Juma <ismael@juma.me.uk>

Closes #1368 from rajinisivaram/minor-channelclose

(cherry picked from commit e20eba9)
Signed-off-by: Ismael Juma <ismael@juma.me.uk>
@ijuma
Copy link
Contributor

ijuma commented May 11, 2016

@rajinisivaram, one thing I was wondering is that we don't propagate the exception in SslTransportLayer, but we do in PlaintextTransportLayer. Thoughts?

@ijuma
Copy link
Contributor

ijuma commented May 11, 2016

Also, in KafkaChannel.close(), we don't close the authenticator if the transport layer throws an exception, which also looks like a bug.

@ijuma
Copy link
Contributor

ijuma commented May 11, 2016

Maybe we could handle both issues in another PR?

@rajinisivaram
Copy link
Contributor Author

@ijuma Thank you for merging so quickly! Yes, I will submit another PR to fix the two issues you mentioned.

gfodor pushed a commit to AltspaceVR/kafka that referenced this pull request Jun 3, 2016
Author: Rajini Sivaram <rajinisivaram@googlemail.com>

Reviewers: Ismael Juma <ismael@juma.me.uk>

Closes apache#1368 from rajinisivaram/minor-channelclose
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants