-
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
MIGRATIONS-1294: Improve remaining space checks for Capture Proxy #286
MIGRATIONS-1294: Improve remaining space checks for Capture Proxy #286
Conversation
Signed-off-by: Tanner Lewis <[email protected]>
Signed-off-by: Tanner Lewis <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #286 +/- ##
============================================
+ Coverage 61.93% 62.16% +0.22%
- Complexity 617 620 +3
============================================
Files 82 82
Lines 3129 3137 +8
Branches 292 292
============================================
+ Hits 1938 1950 +12
+ Misses 1014 1011 -3
+ Partials 177 176 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Approved with comments
* This function determines the number of bytes needed to store a TrafficObservation and a closing index for a | ||
* TrafficStream, from the provided input. | ||
*/ | ||
public static int bytesNeededForObservationAndClosingIndex(int observationContentSize, int flushes) { |
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.
It took me a couple minutes to understand why you cared about (the number of) "flushes". Can you rename this to something like numberOfTrafficStreamsSoFar or something similar. When I think of flush, I don't necessarily think of it as having an observable side effect like what you're looking for.
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.
Sure adjusted
@@ -32,7 +32,7 @@ | |||
* into the defined Protobuf format {@link org.opensearch.migrations.trafficcapture.protos.TrafficStream}, and then write | |||
* this formatted data to the provided CodedOutputStream. | |||
* | |||
* Commented throughout the class are example markers such as (i.e. 1: "1234ABCD") which line up with the textual | |||
* Commented throughout the class are example markers such as (e.g. 1: "1234ABCD") which line up with the textual |
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.
lol - you're right. thanks for correcting the abbreviations :)
if (getOrCreateCodedOutputStream().spaceLeft() < CodedOutputStreamSizeUtil.bytesNeededForObservationAndClosingIndex( | ||
observationContentSize, numFlushesSoFar + 1)) { |
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 please reformat this? This is the hardest to read line break I've seen in a long while. Maybe break after '<' and/or put the brace on the next line.
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.
Yeah this came out a bit ugly, should be a good bit clearer to read now
@@ -155,14 +156,41 @@ public void testBasicDataConsistencyWhenChunking() throws IOException, Execution | |||
Assertions.assertEquals(packetData, reconstructedData); | |||
} | |||
|
|||
@Test | |||
public void testCloseAfterWriteWillFlushWhenSpaceNeeded() throws IOException, ExecutionException, InterruptedException { |
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.
nit: "testCloseObservation..." would be clearer. Sounds like you might have meant to close a stream
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.
Adjusted
Signed-off-by: Tanner Lewis <[email protected]>
Description
This change primarily ensures that every observation we attempt to add to a TrafficStream performs a space check of the CodedOuputStream initially to ensure that there is ample space to store the observation and a closing index, or otherwise flush the stream so that space is available.
Beyond this, there is also some cleaning up of EndOfSegmentIndication size calculations as this will now be handled by the observation size check. As well as general naming and structuring to make things a bit cleaner and easier to follow.
Issues Resolved
https://opensearch.atlassian.net/browse/MIGRATIONS-1294
Testing
Unit testing and manual docker testing
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.