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(datastore) release startStopSemaphore when start returns, not when API sync completes #1027

Merged
merged 3 commits into from
Dec 8, 2020

Conversation

richardmcclellan
Copy link
Contributor

@richardmcclellan richardmcclellan commented Dec 5, 2020

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@richardmcclellan richardmcclellan requested a review from a team December 5, 2020 16:00
Copy link
Contributor

@jamesonwilliams jamesonwilliams left a comment

Choose a reason for hiding this comment

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

Regarding thread safety and synchronization of concurrent calls:

If I understand correctly, all of it is now handled by the Semaphore? And so we don't need to run the transitions on a Schedulers.single(), and we don't need to throw synchronized on any method signatures?

@richardmcclellan
Copy link
Contributor Author

Regarding thread safety and synchronization of concurrent calls:
If I understand correctly, all of it is now handled by the Semaphore? And so we don't need to run the transitions on a Schedulers.single(), and we don't need to throw synchronized on any method signatures?

I'm going to take another closer look at this. Before, I think the semaphore handled everything, and the synchronized methods were unnecessary. Now, we may actually be able to get rid of the semaphore and just have synchronized methods. I'm going to update the logic for stopping on errors and retrying first though, in my next PR.

@richardmcclellan richardmcclellan force-pushed the rm/orchestrator branch 7 times, most recently from cf184a1 to 2a66de7 Compare December 7, 2020 18:09
@richardmcclellan
Copy link
Contributor Author

@jamesonwilliams I think I've addressed all of your comments, so this is ready for review again. I have another PR coming after this one to fix the "crash when going offline" bug, and also to potentially remove the semaphore completely.

Copy link
Contributor

@jamesonwilliams jamesonwilliams left a comment

Choose a reason for hiding this comment

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

Nice cleanup!

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.

Unable to acquire orchestrator lock. Transition currently in progress.
2 participants