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: Adding configurable property to handle double delete events correctly #347

Merged

Conversation

ojhagaurov
Copy link
Contributor

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:

@Override
  public Boolean delete(ConsumerKeyInput key) {
    consumerView.get(key.asConsumerKey()).ifPresent(consumerService::delete);
    return true;
  }

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

  • CHANGELOG.md updated
  • Reviewer assigned
  • PR assigned (presumably to submitter)
  • Labels added (enhancement, bug, documentation)

Comment on lines 65 to 67
if(checkExistEnabled){
consumerBindingView.get(key.asConsumerBindingKey()).ifPresent(consumerBindingService::delete);
}else {
Copy link
Contributor

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.

Suggested change
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);
Copy link
Contributor

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() {
Copy link
Contributor

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?

@jamespfaulkner jamespfaulkner merged commit 55fb7db into ExpediaGroup:master Jan 23, 2024
1 check passed
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.

2 participants