-
Notifications
You must be signed in to change notification settings - Fork 30
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
Enhancements in StreamChannelConnectionCaptureSerializer #494
Enhancements in StreamChannelConnectionCaptureSerializer #494
Conversation
Signed-off-by: Andre Kurait <[email protected]>
… bufferSize Signed-off-by: Andre Kurait <[email protected]> Signed-off-by: Andre Kurait <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #494 +/- ##
============================================
+ Coverage 73.49% 73.53% +0.04%
- Complexity 1180 1181 +1
============================================
Files 124 124
Lines 4886 4886
Branches 439 439
============================================
+ Hits 3591 3593 +2
+ Misses 1000 999 -1
+ Partials 295 294 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
*/ | ||
@Slf4j | ||
public class StreamChannelConnectionCaptureSerializer<T> implements IChannelConnectionCaptureSerializer<T> { | ||
|
||
private static final int MAX_ID_SIZE = 96; | ||
private static final int MAX_ID_SIZE = 100; |
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.
@gregschohn, let me know if we should increase this as 100 is exactly what we expect out of our realIds
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.
I don't have a strong opinion. If I were doing this code from scratch, I'd probably put an assertion that I could create a new CodedOutputStream from the factory that was able to write the boilerplate stuff - all within a lambda inside the assert statement.
Signed-off-by: Andre Kurait <[email protected]> Signed-off-by: Andre Kurait <[email protected]>
906ed02
to
ff23111
Compare
…rializer Signed-off-by: Andre Kurait <[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.
I'd like to see the mockito test get dropped & the code that it was testing improve. I'd recommend filing an issue to improve it & leave that dragon sleep for the moment.
...a/org/opensearch/migrations/trafficcapture/StreamChannelConnectionCaptureSerializerTest.java
Outdated
Show resolved
Hide resolved
...a/org/opensearch/migrations/trafficcapture/StreamChannelConnectionCaptureSerializerTest.java
Outdated
Show resolved
Hide resolved
*/ | ||
@Slf4j | ||
public class StreamChannelConnectionCaptureSerializer<T> implements IChannelConnectionCaptureSerializer<T> { | ||
|
||
private static final int MAX_ID_SIZE = 96; | ||
private static final int MAX_ID_SIZE = 100; |
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.
I don't have a strong opinion. If I were doing this code from scratch, I'd probably put an assertion that I could create a new CodedOutputStream from the factory that was able to write the boilerplate stuff - all within a lambda inside the assert statement.
...a/org/opensearch/migrations/trafficcapture/StreamChannelConnectionCaptureSerializerTest.java
Outdated
Show resolved
Hide resolved
...a/org/opensearch/migrations/trafficcapture/StreamChannelConnectionCaptureSerializerTest.java
Outdated
Show resolved
Hide resolved
df51fa9
to
f42650a
Compare
…cases Signed-off-by: Andre Kurait <[email protected]>
f42650a
to
bb669d8
Compare
Description
This pull request introduces two key enhancements to the StreamChannelConnectionCaptureSerializer:
-ea
jdk flag.-ea
.Issues Resolved
Is this a backport? If so, please add backport PR # and/or commits #
Testing
-enableassertions
with OSB --test script and doc count checkCheck 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.