-
Notifications
You must be signed in to change notification settings - Fork 20
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
refactoring registration and provider service to make it resilient for conflict errors #200
Conversation
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.
cool, thanks for the PR, it looks good to me
as you are at it, could you also add the same use of the x-conflicting-id
header in ProviderService.createOrUpdateProvider(..)
?
…m get provider call
@francoisledroff : please approve |
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.
a few small requests:
return updateProvider(providerId,providerInputModel); | ||
} catch (ConflictException e){ | ||
String conflictingProviderId = e.getConflictingId(); | ||
logger.warn("Another provider (id: `{}` ) exist with conflict due to {}, trying to update it ...", |
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.
logger.info
would be enough, don't you think ?
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.
ok
return createRegistration(registrationCreateModelBuilder); | ||
} catch (ConflictException ex) { | ||
String conflictingRegistrationId = ex.getConflictingId(); | ||
logger.warn("Another registration (id `{}`) exists with conflict due to {}. Trying to update it...", conflictingRegistrationId, ex.getMessage()); |
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.
logger.info
would be enough, don't you think ?
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.
ok
@@ -51,7 +51,7 @@ public void createGetDeleteJournalRegistration() { | |||
try { | |||
providerId = providerServiceTester.createOrUpdateProvider(TEST_EVENT_PROVIDER_LABEL, | |||
TEST_EVENT_CODE).getId(); | |||
Registration registration = createJournalRegistration(TEST_REGISTRATION_NAME, providerId, | |||
Registration registration = createOrUpdateJournalRegistration(TEST_REGISTRATION_NAME, providerId, |
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.
may be we could test both path ?
using createOrUpdate
once and then createOrUpdate
again ... once created
here and may be in ProviderServiceIntegrationTest
too
wdyt ?
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.
I didn't get this
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.
…d update flows without any conflict eexception
@francoisledroff : how can we retrigger the integration test? Locally it is getting passed. |
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.
one small request:
() -> registrationService.findById(finalRegistrationId)); | ||
|
||
// covering the update path | ||
registration = createOrUpdateJournalRegistration(TEST_REGISTRATION_NAME, providerId, |
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.
could you add the same coverage for the ProviderService
pretty please ?
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.
Well,I have done this here in the ProviderServiceIntegrationTest
, is there any other place I need to do that you meant?
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.
oh I did not catch this, thanks !
String updatedProviderDescription = "updated Provider Description"; | ||
Optional<Provider> updatedProvider = providerService.updateProvider(providerId, | ||
getTestProviderInputModelBuilder(TEST_EVENT_PROVIDER_LABEL) | ||
Provider updatedProvider = createOrUpdateProvider(getTestProviderInputModelBuilder(TEST_EVENT_PROVIDER_LABEL) |
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.
here
Description
See issue description
Related Issue
#199
https://jira.corp.adobe.com/browse/CI-6365
Motivation and Context
Make our e2e more resilient
How Has This Been Tested?
Using
aio-events-itests
running locallyScreenshots (if appropriate):
Types of changes
Checklist: