Skip to content

Conversation

@kennknowles
Copy link
Member

In the SDK the path taken by Throwables.propagate is always statically known, and the inlined logic is more explicit and readable:

  • If an exception e is already a checked exception, Throwables.propagate(e)
    is the same as throw new RuntimeException(e).
  • If an exception e is already a RuntimeException or Error,
    Throwables.propagate(e) is the same as throw e.

@kennknowles
Copy link
Member Author

R: @lukecwik

It also looks like there might be some poor handling of an InterruptedException here. LMK what you think.

In the SDK the path taken by Throwables.propagate is always statically
known, and the inlined logic is more explicit and readable:

 - If an exception e is already a checked exception, Throwables.propagate(e)
   is the same as `throw new RuntimeException(e)`.
 - If an exception e is already a RuntimeException or Error,
   Throwables.propagate(e) is the same as `throw e`.
@kennknowles kennknowles force-pushed the throwables-propagate branch from 5c13f33 to 8a8a3d4 Compare March 25, 2016 02:47
if (currentLatch > 0) {
countDownLatches[currentLatch - 1].countDown();
}
} catch (InterruptedException e) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add Thread.currentThread().interrupt() before the throws

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. (just below the diff)

@dhalperi
Copy link
Contributor

@kennknowles @lukecwik ping?

@kennknowles
Copy link
Member Author

pong

@lukecwik
Copy link
Member

LGTM
Waiting on tests to finish running.

@asfgit asfgit closed this in 061e6b5 Mar 31, 2016
@kennknowles kennknowles deleted the throwables-propagate branch April 19, 2016 17:23
iemejia referenced this pull request in iemejia/beam Jan 12, 2018
mareksimunek pushed a commit to mareksimunek/beam that referenced this pull request May 9, 2018
* apache#70 Add (failing) proof-of-problem test

* apache#70 [euphoria-flink] Fix trigger state clean-up of merged windows

* apache#70 [euphoria-inmem] Limit window-trigger-test to stream processing only
dmvk pushed a commit to dmvk/beam that referenced this pull request May 15, 2018
* apache#70 Add (failing) proof-of-problem test

* apache#70 [euphoria-flink] Fix trigger state clean-up of merged windows

* apache#70 [euphoria-inmem] Limit window-trigger-test to stream processing only
mareksimunek pushed a commit to seznam/beam that referenced this pull request Jul 9, 2018
* apache#70 Add (failing) proof-of-problem test

* apache#70 [euphoria-flink] Fix trigger state clean-up of merged windows

* apache#70 [euphoria-inmem] Limit window-trigger-test to stream processing only
dmvk pushed a commit to seznam/beam that referenced this pull request Aug 17, 2018
* apache#70 Add (failing) proof-of-problem test

* apache#70 [euphoria-flink] Fix trigger state clean-up of merged windows

* apache#70 [euphoria-inmem] Limit window-trigger-test to stream processing only
dmvk pushed a commit to seznam/beam that referenced this pull request Oct 5, 2018
* apache#70 Add (failing) proof-of-problem test

* apache#70 [euphoria-flink] Fix trigger state clean-up of merged windows

* apache#70 [euphoria-inmem] Limit window-trigger-test to stream processing only
pl04351820 pushed a commit to pl04351820/beam that referenced this pull request Dec 20, 2023
towards apache#65

this PR is staged on top of apache#69 as refactor changes are cross-class; I encourage reviewing in sequential order
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