-
Notifications
You must be signed in to change notification settings - Fork 27
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
[RFS] Improved RFS Worker exception logging #676
Conversation
Signed-off-by: Chris Helma <[email protected]>
Signed-off-by: Chris Helma <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #676 +/- ##
=============================================
- Coverage 75.51% 63.44% -12.08%
- Complexity 1422 1561 +139
=============================================
Files 150 216 +66
Lines 6147 8414 +2267
Branches 561 758 +197
=============================================
+ Hits 4642 5338 +696
- Misses 1135 2674 +1539
- Partials 370 402 +32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
@chelma Thanks for making progress in this space, I've got some overall concerns about the data access pattern associated with what the CMS client is returning captured in OpenSearchCmsClient.java
Since this PR is focused around exception logging - lets add test case to make sure the exception logging is propagating as you expect. It might be worthwhile to refactor how some of these methods are configured and called to make test cases of this nature easier to author.
Signed-off-by: Chris Helma <[email protected]>
Signed-off-by: Chris Helma <[email protected]>
Still to-do - test cases around logging exceptions. |
Signed-off-by: Chris Helma <[email protected]>
@peternied OK - Refactored a bit more and added some tests. Curious to see what you think now. |
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.
@chelma Thanks for updating the interfaces, these are looking better; however, I am seeing some issues in repeating patterns in how this code works - I've called out what looks like types of usage that look like they need adjustment and then applied throughout the codebase.
Let me know if you've got questions
if (createdEntry.isPresent()) { | ||
return Optional.of(OpenSearchCmsEntry.Snapshot.fromJson(createdEntry.get())); | ||
} else { | ||
return null; | ||
return Optional.empty(); | ||
} |
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.
Operations on Optional<>
will not be handled unless there is a value inside them, so this block can be simplify this greatly by using map
to the following:
return createdEntry.map(OpenSearchCmsEntry.Snapshot::fromJson)
Optional<ObjectNode> document = client.getDocument(CMS_INDEX_NAME, CMS_SNAPSHOT_DOC_ID); | ||
if (document.isEmpty()) { | ||
return Optional.empty(); | ||
} | ||
return OpenSearchCmsEntry.Snapshot.fromJsonString(response.body); | ||
|
||
ObjectNode sourceNode = (ObjectNode) document.get().get("_source"); | ||
return Optional.of(OpenSearchCmsEntry.Snapshot.fromJson(sourceNode)); |
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.
Map simplifies this method greatly too:
return client.getDocument(CMS_INDEX_NAME, CMS_SNAPSHOT_DOC_ID)
.map(document -> (ObjectNode) document.get("_source"))
.map(OpenSearchCmsEntry.Snapshot::fromJson);
} | ||
return OpenSearchCmsEntry.Snapshot.fromJsonString(response.body); | ||
|
||
ObjectNode sourceNode = (ObjectNode) document.get().get("_source"); |
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 we alter the object model so this casting isn't needed?
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.
AFAIK, not really; this is a Jackson thing. When you .get()
a field on an ObjectNode
, it returns a JsonNode
(its parent class). The returned object might be an ObjectNode
, or it might not, depending on what that field contains. It feels like this is the right spot to determine if this is an ObjectNode
or something like a ArrayNode
.
@@ -99,11 +108,16 @@ public RestClient.Response registerSnapshotRepo(String repoName, ObjectNode sett | |||
.doOnError(e -> logger.error(e.getMessage())) | |||
.retryWhen(Retry.backoff(3, Duration.ofSeconds(1)).maxBackoff(Duration.ofSeconds(10))) | |||
.block(); | |||
|
|||
return Optional.of(settings); |
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.
This doesn't seem correct, if the registration fails, the method should not return an instance of the settings - but of an empty object, no?
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.
You're correct - this behavior is wrong, and doesn't match the comment for the method.
Signed-off-by: Chris Helma <[email protected]>
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 much better, great work @chelma!
Description
Issues Resolved
Testing
ReindexFromSnapshot.java
all-in-one script against the docker compose test setupCheck List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.