-
Notifications
You must be signed in to change notification settings - Fork 12
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
EVA-3554 Pass application instance id to generate accession #87
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.
Looks good.
Left a few cosmetic comments
@@ -46,7 +46,7 @@ public interface AccessioningService<MODEL, HASH, ACCESSION> { | |||
* @return List of wrapper objects containing the accessioned objects and their associated accessions and hashes | |||
* @throws AccessionCouldNotBeGeneratedException when accession could not be generated | |||
*/ | |||
List<GetOrCreateAccessionWrapper<MODEL, HASH, ACCESSION>> getOrCreate(List<? extends MODEL> messages) | |||
List<GetOrCreateAccessionWrapper<MODEL, HASH, ACCESSION>> getOrCreate(List<? extends MODEL> messages, String applicationInstanceId) |
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.
Can you update the method comment to explain what is the applicationInstanceId
and why we're passing 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.
Also I'm wondering if we should avoid reusing the variable applicationInstanceId
in case there are still beans that would populate it from the properties file.
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.
Added the comment.
As These are parameter variables and not instance variables of a class, these would not get populated automatically.
@@ -127,7 +124,7 @@ public synchronized long[] generateAccessions(int numAccessionsToGenerate) | |||
* | |||
* @param totalAccessionsToGenerate | |||
*/ | |||
private synchronized void reserveNewBlocksUntilSizeIs(int totalAccessionsToGenerate) { | |||
private synchronized void reserveNewBlocksUntilSizeIs(int totalAccessionsToGenerate, String applicationInstanceId) { |
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.
Same here can we update the comment ?
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.
done
No description provided.