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

ARUHA-1693: Added functionality to delete subscriptions together with event-type (if feature is toggled); #896

Merged
merged 5 commits into from Jun 25, 2018

Conversation

3 participants
@v-stepanov
Member

v-stepanov commented Jun 22, 2018

Added functionality to delete subscriptions together with event-type (if feature is toggled)

Zalando ticket : ARUHA-1693

Description

On test environments we may allow users to delete event-types together with subscriptions

Review

  • Tests
  • Documentation
  • CHANGELOG

Deployment Notes

We should toggle this feature to make it work

@codecov-io

This comment has been minimized.

codecov-io commented Jun 22, 2018

Codecov Report

Merging #896 into master will increase coverage by <.01%.
The diff coverage is 68.18%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #896      +/-   ##
============================================
+ Coverage     54.08%   54.09%   +<.01%     
- Complexity     1754     1760       +6     
============================================
  Files           316      316              
  Lines          9663     9680      +17     
  Branches        886      887       +1     
============================================
+ Hits           5226     5236      +10     
- Misses         4114     4121       +7     
  Partials        323      323
Impacted Files Coverage Δ Complexity Δ
...g/zalando/nakadi/service/FeatureToggleService.java 53.12% <100%> (+1.51%) 0 <0> (ø) ⬇️
...a/org/zalando/nakadi/service/EventTypeService.java 75.59% <66.66%> (-1.31%) 53 <7> (+6)

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 4a7cbed...07427f7. Read the comment docs.

}
@SuppressWarnings("unchecked")
private Multimap<TopicRepository, String> deleteEventTypeWithSubscriptions(final String eventType) {

This comment has been minimized.

@rcillo

rcillo Jun 25, 2018

Member

@v-stepanov why not delete the event type inside the transaction? Since there is a database consistency check, it would not be possible to commit the transaction if a subscription was created concurrently, right? Then it would fail the request and the caller could try to delete again manually. Attempting to perform the retry by yourself doesn't seem to cover all the possible race conditions, since it theoretically could retry for 1000 miliseconds and still fail.

But, theory apart, I think the way you did works. So I'm just commenting to know if you considered this other option and why you decided to go this way.

This comment has been minimized.

@v-stepanov

v-stepanov Jun 25, 2018

Member

I already run everything from within a transaction. (inside transactionTemplate.execute(...))
But I think I handle errors in a wrong way. You are right that if the new subscription is created - the transaction will fail: not in the place where ET is removed but in the place of transaction execution. Ok, I will fix it.

This comment has been minimized.

@v-stepanov

v-stepanov Jun 25, 2018

Member

@rcillo I changed it. I remove the retry as it anyway doesn't give 100% guarantee of deletion. So I think the user should retry.
07427f7

throw new ConflictException("Can't remove event type " + eventType
+ ", as it has subscriptions");
}
return transactionTemplate.execute(action -> deleteEventType(eventType));

This comment has been minimized.

@rcillo

rcillo Jun 25, 2018

Member

Do you need to do it in a transaction?

This comment has been minimized.

@v-stepanov

v-stepanov Jun 25, 2018

Member

It used to run inside a transaction, so I didn't change anything here.
I think it is needed because delete event-type also handles the deletion of timelines.

@rcillo

This comment has been minimized.

Member

rcillo commented Jun 25, 2018

👍

@v-stepanov

This comment has been minimized.

Member

v-stepanov commented Jun 25, 2018

deploy validation please

@v-stepanov

This comment has been minimized.

Member

v-stepanov commented Jun 25, 2018

👍

@v-stepanov v-stepanov merged commit ea3aa74 into master Jun 25, 2018

8 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
aruha/jenkins Build finished.
Details
codecov/patch 68.18% of diff hit (target 54.08%)
Details
codecov/project 54.09% (+<.01%) compared to 4a7cbed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
zappr Approvals: @rcillo, @v-stepanov.
zappr/pr/specification PR has passed specification checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment