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

fix: Return an error on publish if the publisher's state is not RUNNING #81

Merged
merged 8 commits into from
Jun 8, 2020

Conversation

tmdiep
Copy link
Collaborator

@tmdiep tmdiep commented May 26, 2020

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes internal issue b/156525288.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 26, 2020
@codecov
Copy link

codecov bot commented May 26, 2020

Codecov Report

Merging #81 into master will increase coverage by 0.09%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #81      +/-   ##
============================================
+ Coverage     60.11%   60.21%   +0.09%     
  Complexity      388      388              
============================================
  Files            90       90              
  Lines          2046     2046              
  Branches        170      171       +1     
============================================
+ Hits           1230     1232       +2     
+ Misses          724      722       -2     
  Partials         92       92              
Impacted Files Coverage Δ Complexity Δ
.../cloud/pubsublite/internal/wire/PublisherImpl.java 78.15% <75.00%> (+1.68%) 20.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb35590...5f57076. Read the comment docs.

@tmdiep
Copy link
Collaborator Author

tmdiep commented May 26, 2020

Not sure if this is the best solution.
I didn't call onPermanentError() because the publisher may already be in a failed or terminated state.

@dpcollins-google
Copy link
Collaborator

Not sure if this is the best solution.
I didn't call onPermanentError() because the publisher may already be in a failed or terminated state.

This should be okay. ProxyService will handle this gracefully, please add onPermanentError.

@tmdiep
Copy link
Collaborator Author

tmdiep commented Jun 2, 2020

I didn't call onPermanentError() because the publisher may already be in a failed or terminated state.

This should be okay. ProxyService will handle this gracefully, please add onPermanentError.

Done. PTAL at revised implementation in PublisherImpl.

@@ -236,6 +237,13 @@ void flushToStream() {
}
}

@VisibleForTesting
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove this method. You can check the permanent error handlerr to determine if a permanent error occurred.

Copy link
Collaborator Author

@tmdiep tmdiep Jun 2, 2020

Choose a reason for hiding this comment

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

Yeah, that was the first thing I tried, but the listeners don't get called. I traced through and think it's because AbstractService::notifyFailed just throws an IllegalStateException when the state is New or Terminated, and doesn't transition to Failed. Since there's no state transition, the listeners aren't called.

I couldn't see another way to verify that PublisherImpl had permanently shutdown, so added isShutdown(). But could just omit that check in the test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Put it into STARTING or STOPPING by making the startAsync or stopAsync method of the RetryingConnection block on a CountdownLatch. Then try to publish to trigger this case, and then let the method proceed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, CountDownLatch is useful. I looked into your suggestion and noticed that all of our doStarts are synchronous. The only scenario that really triggers the original bug (b/156525288) is when the user publishes before calling startAsync, and so left the test to check NEW.

I removed the isShutdown() and just verified that the service has failed permanently by checking that an IllegalStateException is thrown if the user then tries to start the service. This occurs if they try to start PublisherImpl or any proxy such as WrappingPublisher.

Copy link
Collaborator

@dpcollins-google dpcollins-google left a comment

Choose a reason for hiding this comment

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

See comments

@dpcollins-google dpcollins-google merged commit 74d61fd into master Jun 8, 2020
@dpcollins-google dpcollins-google deleted the tmdiep-patch-2 branch June 8, 2020 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants