-
Notifications
You must be signed in to change notification settings - Fork 54
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: Adding configurable property to handle double delete events correctly #347
fix: Adding configurable property to handle double delete events correctly #347
Conversation
if(checkExistEnabled){ | ||
consumerBindingView.get(key.asConsumerBindingKey()).ifPresent(consumerBindingService::delete); | ||
}else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Formatting is a bit funky.
if(checkExistEnabled){ | |
consumerBindingView.get(key.asConsumerBindingKey()).ifPresent(consumerBindingService::delete); | |
}else { | |
if (checkExistEnabled) { | |
consumerBindingView.get(key.asConsumerBindingKey()).ifPresent(consumerBindingService::delete); | |
} else { |
Boolean result = consumerBindingMutation.delete(key); | ||
verify(consumerBindingService, times(1)).delete(any()); | ||
verify(consumerBindingView, times(1)).get(any()); | ||
assert (result); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Should use JUnit/Hamcrest assertions in tests rather than assert
keyword (which is typically used in production code, similar to Preconditions)
} | ||
|
||
@Test | ||
public void deleteWithCheckExistEnabled() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also have the case here where checkExistEnabled = true
, but there isn't an existing entity, ensuring that delete is not called?
Configurable property to enable or disable checking if entity exists in EntityView during delete.
We need a way of servicing deletes when the EntityView thinks they are already deleted.
Details
Currently, the OSS project will not allow us to delete entities that don't exist in the EntityView. For example, the implementation of delete from ConsumerMutationImpl includes an ifPresent check:
Eg: When no tombstone for the original schema key id during handling of double delete events, the identified entities will not exist in the EntityView and hence it will be impossible to delete them.
Changed
entityView.exist.check.enabled property to decide if entity exists in EntityView during delete operation.
PR Checklist Forms