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

Swap PingThread#dead* abstraction #53

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@lacostej
Copy link
Member

commented May 22, 2015

I noticed that the way PingThread#onDead(*) methods are deprecated/implemented could cause clients to not implement the method that gives the most feedback.

So here's a new implementation that

  1. force subclasses to implement the most meaningful method
  2. in a separate commit, get rid of the old method altogether

Existing clients should have to implement the new method anyway. The old one isn't used (except maybe by implementing clients?) and the proposed default implementation might give more feedback if ever called.

I am unable to run full tests due to a PipeTest hang when running mvn test.

lacostej added some commits May 22, 2015

Replace deprecated PingThread#onDead() abstract implementation with d…
…efault one to make it easier for new implementations
@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented May 22, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@@ -134,16 +134,16 @@ private void ping() throws IOException, InterruptedException {
* and provide a fallback behaviour to be backward compatible with earlier version of remoting library.
*/
@Deprecated
protected abstract void onDead();
protected void onDead() {
onDead(new RuntimeException("No cause"));

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jun 9, 2015

Member

What about a @CheckForNull method? seems to be a more convenient solution

This comment has been minimized.

Copy link
@lacostej

lacostej Jun 11, 2015

Author Member

I am not sure what you mean here! :)

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Oct 1, 2015

Member

Sorry for missing the response. My idea was to put the null check logic to protected void onDead(Throwable diagnosis), but it's need

protected void onDead(Throwable diagnosis) {
onDead(); // fall back
}
protected abstract void onDead(Throwable diagnosis);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Oct 1, 2015

Member

This is a public class, which could be used somewhere else. Probably you could just add a non-fatal default implementation. E.g.:

protected void onDead(@CheckForNull Throwable diagnosis) {
    System.err.println("Ping failed. Cause: " + diagnosis);
    if (diagnosis != null) {
        diagnosis.printStackTrace(System.err);
    }
}
@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Oct 2, 2015

retriggered the CI

@oleg-nenashev oleg-nenashev self-assigned this Nov 12, 2015

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Nov 12, 2015

The PR needs to be polished a bit. I'll try to handle it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.