Feat/214 dataspace discovery service handle multiple edc urls received for bpn #711
Conversation
…stest non-failing result while ignoring the others
…rs-registry-client
…multiple-EDC-Urls-received-for-BPN
reason: returning Optional requires too many changes in existing code when ResultFinder is used later
…multiple-EDC-Urls-received-for-BPN
…istryService - EndpointDataForConnectorsService returns multiple EndpointDataReferences now - These are handled in DecentralDigitalTwinRegistryService.fetchShellDescriptors and by selecting the results of the fastest
…scovery-Service-handle-multiple-EDC-Urls-received-for-BPN
|
CHANGELOG file was not updated! Make sure to include important changes. |
…rAsset in combination with the actual data request
…scovery-Service-handle-multiple-EDC-Urls-received-for-BPN
…scovery-Service-handle-multiple-EDC-Urls-received-for-BPN # Conflicts: # irs-registry-client/src/test/java/org/eclipse/tractusx/irs/registryclient/DefaultConfigurationTest.java
switch log.debug to info because debug does not seem to get logged and add special marker "#214@" to filter the logs for debugging the story
…scovery-Service-handle-multiple-EDC-Urls-received-for-BPN
…multiple-EDC-Urls-received-for-BPN # Conflicts: # CHANGELOG.md
…multiple-EDC-Urls-received-for-BPN
This commit corrects the glossary sort order and adds some missing definitions, explanations and links.
…ug infos Moved logging from helper class to business class because this way the log entry shows the business class and not the helper class.
…scovery-Service-handle-multiple-EDC-Urls-received-for-BPN
|
|
||
| fastestResultPromise.completeExceptionally(new CompletionExceptions("None successful", exceptions)); | ||
| } else { | ||
| log.debug("Completing"); |
There was a problem hiding this comment.
I think you could make this log more verbose, what is completing or something, this may not to meaningful
There was a problem hiding this comment.
difficult because of the CompletableFuture we don't have any context here. We could log the value of the request, but this might contain stuff that may not be logged and might be quite verbose?!
|
|
||
| if (notFinishedByOtherFuture && currentFutureSuccessful) { | ||
|
|
||
| // first future that completes successfully completes the overall future |
There was a problem hiding this comment.
I would not recommend describing code behaviour as comments, we could add more description in below log or wrap it in named method (something like First future completed successfully, complitting overall future.
refs:
comment rules point 5. -> https://gist.github.com/wojteklu/73c6914cc446146b8b533c0988cf8d29
https://medium.com/codex/clean-code-comments-833e11a706dc
There was a problem hiding this comment.
Actually this is not just saying what the code does but WHY. In my opinion this makes it easier to understand why we have two completable futures here: the actual data retrieval future and the overall future that is just there to complete as soon as the first future completes successfully. But obviously the comment did not transport this. Therefore I renamed the overall promise, removed the comment here and added one when the overall promise is declared that describes the purpose of the overall promise.
| log.info("Found {} shell(s) for {} key(s)", collectedShells.size(), keys.size()); | ||
| return collectedShells; | ||
|
|
||
| final var watch = new StopWatch(); |
There was a problem hiding this comment.
how about making stopwatch instance scope instead of instantiating it every time method is called?
"Note that a StopWatch is reusable. That is, you can call start() and stop() in succession and the getElapsedTime() method will refer to the time since the most recent start() call."
There was a problem hiding this comment.
This is asynchronous code. Therefore the measured times would not be correct because two threads would use the same stopwatch.
| try { | ||
| return fetchShellDescriptors(entry, calledEndpoints); | ||
| } catch (RuntimeException e) { | ||
| // catching generic exception is intended here, |
There was a problem hiding this comment.
same here, instead of commenting out consider wrapping it in named method that would describe finishing of a Job
| } else { | ||
| log.info("Retrieved shellId {} for globalAssetId {}", aaShellIdentification, key); | ||
|
|
||
| final var watch = new StopWatch(); |
| return lookupShellIds(bpn, endpointDataReferenceFutures); | ||
|
|
||
| } catch (RuntimeException e) { | ||
| // catching generic exception is intended here, |
There was a problem hiding this comment.
Actually catching the generic RuntimeException is a bad practice normally. The comment describes why it is intended and valid in this special case.
| public List<CompletableFuture<EndpointDataReference>> createFindEndpointDataForConnectorsFutures( | ||
| final List<String> connectorEndpoints) { | ||
|
|
||
| final var watch = new StopWatch(); |
There was a problem hiding this comment.
LGTM, please consider the changes requested by @ds-psosnowski
…scovery-Service-handle-multiple-EDC-Urls-received-for-BPN
|
dsmf
deleted the
feat/214-Dataspace-Discovery-Service-handle-multiple-EDC-Urls-received-for-BPN
branch
Chore: Update dependency
story: eclipse-tractusx#214
Please note the following comments: