Skip to content

Conversation

@rxin
Copy link
Contributor

@rxin rxin commented Apr 22, 2016

What changes were proposed in this pull request?

This is a follow-up to #12557, with the following changes:

  1. Fixes some of the style issues.
  2. Merges Signaling and SignalLogger into a new class called SignalUtils. It was pretty confusing to have Signaling and Signal in one file, and it was also confusing to have two classes named Signaling and one called the other.
  3. Made logging registration idempotent.

How was this patch tested?

N/A.

@rxin
Copy link
Contributor Author

rxin commented Apr 22, 2016

cc @jodersky

}

if(escalate) {
val escalate = actions.asScala.forall { action => !action() }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you write a short description here on what this is doing? it's not very straightforward (especially the forall and boolean inversion)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, I initially had comments but thought they were overkill since the behaviour can be deducted from the comments in the register() method. I'll add some more info

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how should I add the comments? Just post them here and you'll update the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good

Copy link
Member

@jodersky jodersky Apr 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/**
     * Called when this handler's signal is received. Note that if the same signal is received
     * before this method returns, it is escalated to the previous handler.
     */
    override def handle(sig: Signal): Unit = {
      // register old handler, will receive incoming signals while this handler is running
      Signal.handle(signal, prevHandler)

      // run all actions, escalate to parent handler if no action says the signal was caught
      // (i.e. all actions return false) 
      val escalate = actions.asScala.forall { action => !action() }
      if (escalate) {
        prevHandler.handle(sig)
      }

      // re-register this handler
      Signal.handle(signal, this)
    }

is this better? I can add even more if you prefer

@rxin
Copy link
Contributor Author

rxin commented Apr 22, 2016

OK updated.

@SparkQA
Copy link

SparkQA commented Apr 22, 2016

Test build #56673 has finished for PR 12605 at commit fa1c83a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Apr 22, 2016

Test build #56682 has finished for PR 12605 at commit 9a8f93c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@rxin
Copy link
Contributor Author

rxin commented Apr 22, 2016

Merging in master.

@asfgit asfgit closed this in c089c6f Apr 22, 2016
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.

3 participants