More context on throws #453
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…as being dropped. This can create an error down the line if the underlying implementation changes, even if it works now. I've also removed a getValue() that didn't do anything. Signed-off-by: Greg Schohn <[email protected]>
… RequestKey.toString(). Also pass the request key through ParsedHttpMessagesAsDicts so that the request key is present in the (which I've just realized should be sending back optionals or sneaky throws & letting the callers do this conversion... next change). Signed-off-by: Greg Schohn <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #453 +/- ##
============================================
- Coverage 76.86% 71.16% -5.70%
+ Complexity 1151 1144 -7
============================================
Files 145 121 -24
Lines 5887 4890 -997
Branches 542 432 -110
============================================
- Hits 4525 3480 -1045
- Misses 1080 1138 +58
+ Partials 282 272 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
gregschohn
requested review from
chelma,
kartg,
lewijacn,
mikaylathompson,
okhasawn and
sumobrian
as code owners
Signed-off-by: Greg Schohn <[email protected]> # Conflicts: # TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/datatypes/PojoTrafficStreamKey.java # TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/kafka/KafkaProtobufConsumer.java
sumobrian
approved these changes
Nov 23, 2023
| log.warn("Terminating a TrafficStream reconstruction w/out an accumulated value, " + | ||
| "assuming an empty server interaction and NOT reproducing this to the target cluster."); | ||
| log.atWarn().setMessage("Terminating a TrafficStream reconstruction before data was accumulated " + | ||
| "for " + accumulation.trafficChannelKey + "assuming an empty server interaction and NOT " + |
There was a problem hiding this comment.
add space before 'assuming'.
| log.warn("Putting what may be a bogus value in the output because transforming it " + | ||
| "into json threw an exception"); | ||
| // TODO - this isn't a good design choice. | ||
| // We should follow through with the spirit of this class and leave thes as empty optional values |
| @@ -171,7 +172,7 @@ private <T> void scheduleOnConnectionReplaySession(ISourceTrafficChannelKey chan | |||
| eventLoop.schedule(task, getDelayFromNowMs(atTime), TimeUnit.MILLISECONDS); | |||
| scheduledFuture.addListener(f->{ | |||
| if (!f.isSuccess()) { | |||
| log.atError().setCause(f.cause()).setMessage(()->"Error scheduling task").log(); | |||
| log.atError().setCause(f.cause()).setMessage(()->"Error scheduling task for "+channelKey).log(); | |||
| @@ -922,7 +923,7 @@ private static String formatWorkItem(DiagnosticTrackableCompletableFuture<String | |||
| } | |||
|
|
|||
| public @NonNull CompletableFuture<Void> shutdown(Error error) { | |||
| log.warn("Shutting down "+this+" because of "+error); | |||
| log.atWarn().setCause(error).setMessage(()->"Shutting down "+this+" because of error").log(); | |||
| * block calls to readNextTrafficStreamChunk() until some time window elapses. This | ||
| * could be a very large window in cases where there were long gaps between recorded | ||
| * requests from the capturing proxy. For example, if a TrafficStream is read and it | ||
| * that stream is scheduled to be run one hour later, readNextTrafficStreamChunk() |
There was a problem hiding this comment.
update: read and if* that stream is scheduled
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Some warning log messages were missing some critical context. I made a sweep through all error/warn messages and added an appropriate amount of context to those that were lacking.
Testing
unit tests
Check 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.