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

Provide idempotency support to handle duplicate maintenance events #1407

Merged
merged 2 commits into from
Dec 4, 2020

Conversation

efeg
Copy link
Collaborator

@efeg efeg commented Dec 3, 2020

This PR resolves #1347.

@efeg efeg requested a review from Lincong December 3, 2020 02:50
Copy link
Contributor

@Lincong Lincong left a comment

Choose a reason for hiding this comment

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

The overall implementation looks good. But I am wondering whether we can consider the following two options.

  1. Use caffeine cache (link). Maybe this use case is simple enough and we do not want to introduce a new dependency for it?

  2. Create a separate class MaintenanceEventCache that includes the cache cleaner, cache size/ time retention, etc. There could be several advantages for doing it.

  • Improve simplicity/readability/modularity of the MaintenanceEventDetector class
  • The concurrency control can happen on the MaintenanceEventCache class level instead of MaintenanceEventDetector level (currently, removeOldEventsFromIdempotenceCache and ensureIdempotency methods are synchronized on the MaintenanceEventDetector level). It makes more sense to make MaintenanceEventCache handle its own concurrent data access.
  • Improve testability since it's more modular

@efeg
Copy link
Collaborator Author

efeg commented Dec 4, 2020

Thanks for the review.

  1. Correct -- adding dependency on something like Caffeine (or Guava) for this simple need would be an overkill.
  2. It makes sense to have a separate class -- please see IdempotenceCache.

@efeg efeg force-pushed the feat/idempotencyMaintenanceEvent branch from 92ab573 to 0ef70ae Compare December 4, 2020 03:14
@efeg efeg merged commit 81f99cc into linkedin:master Dec 4, 2020
efeg added a commit to efeg/cruise-control that referenced this pull request Dec 4, 2020
efeg added a commit to efeg/cruise-control that referenced this pull request Dec 4, 2020
@efeg efeg deleted the feat/idempotencyMaintenanceEvent branch December 4, 2020 03:44
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.

Provide idempotency support to handle duplicate maintenance events
2 participants