-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
Simplify TransportStats assertions in v9 #114700
Conversation
Transport handling times were added in elastic#80581 (8.1), we don't need assertions for version prior to that in 9.0
Pinging @elastic/es-distributed (Team:Distributed) |
…nto v9-transport-stats-assertions
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 would prefer that we clean up the de/serialization code at this point too.
This reverts commit 35a2901.
Pinging @elastic/es-distributed-obsolete (Team:Distributed (Obsolete)) |
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
929dda9
to
bb59b3b
Compare
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 think there's room for further simplification here, or at least better alignment between the serialization and deserialization code - see comment inline.
if (out.getTransportVersion().onOrAfter(TransportVersions.V_8_1_0)) { | ||
assert (inboundHandlingTimeBucketFrequencies.length > 0) == (outboundHandlingTimeBucketFrequencies.length > 0); | ||
assert (inboundHandlingTimeBucketFrequencies.length > 0) == (outboundHandlingTimeBucketFrequencies.length > 0); | ||
if (out.getTransportVersion().before(TransportVersions.TRANSPORT_STATS_HANDLING_TIME_REQUIRED)) { | ||
out.writeBoolean(inboundHandlingTimeBucketFrequencies.length > 0); |
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 match up with the reading code which requires these things to have length HandlingTimeTracker.BUCKET_COUNT
.
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've hardened up the assertions on the bucket frequencies length in efc71fd
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.
LGTM
Again, pretty important to backport this to v9.0
💔 Backport failed
You can use sqren/backport to manually backport by running |
Backports elastic#114700 to 9.0 > Transport handling times were added in elastic#80581 (8.1), we don't need assertions for version prior to that in 9.0
Transport handling times were added in #80581 (8.1), we don't need assertions for version prior to that in 9.0