Skip to content
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

Ensure that we don't pass negative timeInQueue to writeVLong #9662

Merged
merged 1 commit into from
Feb 11, 2015

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Feb 11, 2015

#writeVLong can only serialize positive values, yet this BWC code
in PendingClusterTask passes occational -1 causing assertions to trip.
It also yields completely wrong values ie. if -1 is deserialized it yields
9223372036854775807. This commit ensure that timeInQueue is positive ie.
at least 0

Relates to #8077

Note this PR is against 1.x

`#writeVLong` can only serialize positive values, yet this BWC code
in `PendingClusterTask` passes occational `-1` causing assertions to trip.
It also yields completely wrong values ie. if `-1 is deserialized it yields
`9223372036854775807`. This commit ensure that `timeInQueue` is positive ie.
at least `0`

Relates to elastic#8077
@martijnvg
Copy link
Member

LGTM

Maybe this can problem can occur on other places as well and a helper method on StreamOutput makes sense?

@s1monw
Copy link
Contributor Author

s1monw commented Feb 11, 2015

Maybe this can problem can occur on other places as well and a helper method on StreamOutput makes sense?

I mean we had this problem all over the place but I don't see how a helper can improve it? Maybe we should upgrade the assertion to an exception?

@s1monw s1monw merged commit 1e07ab2 into elastic:1.x Feb 11, 2015
out.writeVLong(timeInQueue);
}
out.writeVLong(Math.max(0, timeInQueue));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation issue?

@jpountz
Copy link
Contributor

jpountz commented Feb 11, 2015

LGTM

@martijnvg
Copy link
Member

I mean we had this problem all over the place but I don't see how a helper can improve it?

Having a small helper method on StreamOutput would prevent repeating this Math.max(...) logic many times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants