Skip to content

Conversation

@bharathkk
Copy link
Contributor

@bharathkk bharathkk commented Sep 28, 2018

The PR addresses the following issues

  • We have few scenarios where there is a race condition between SamzaContainerListener and StreamProcessor that results in incorrect application status being propagated to client

Consider the scenario when the container runs into an exception in run loop and triggers shutdown sequence and at the same time the user triggers a stop on the StreamProcessor. The user request comes in and notices that the container is already shutting down and proceeds to shutdown the job coordinator which in turns shuts down successfully and the processorListener.onStop() is invoked. The container eventually invokes the SamzaContainerListener callback and updates the exception state after the StreamProcessor has finished shutting down.

  • Currently, we only propagate failures to processorListener when containerException is not null. It is possible for the samza container to take longer than task.shutdown.ms to shutdown in which case, we need to propagate a timeout exception to the processorListener as opposed assuming the shutdown was successful.

  • Make container shutdown idempotent since its only setting a boolean flag to true and there is no need to throw an exception on subsequent invocations.
    1. It simplifies the interaction of other components (StreamProcessor) with container and prevents unnecessary check on container state to determine if its safe to call shutdown or not.
    2. Enables to reason about the impact of container state change on StreamProcessor easily since we restrict container interaction w/ StreamProcessor only via SamzaContainerListener.

@bharathkk
Copy link
Contributor Author

@nickpan47 @prateekm @shanthoosh
Please take a look

@shanthoosh
Copy link
Contributor

Looks like there're compilation errors in new tests, please fix them.

@bharathkk bharathkk force-pushed the samza-1832-alternative branch from 0748cfd to 9caa09c Compare October 1, 2018 19:26
@bharathkk bharathkk force-pushed the samza-1832-alternative branch from 9caa09c to 11d577d Compare October 1, 2018 23:43
Copy link
Contributor

@nickpan47 nickpan47 left a comment

Choose a reason for hiding this comment

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

minor comment. lgtm. Thanks!

@asfgit asfgit closed this in 6e5e162 Oct 4, 2018
asfgit pushed a commit that referenced this pull request Oct 8, 2018
Due to interleaved ordering of the PRs (#675 and #673), FaultInjectionTest is broken.

Author: bharathkk <[email protected]>

Reviewers: Shanthoosh Venkatraman <[email protected]>

Closes #700 from bharathkk/fix-trunk
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.

5 participants