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

Fix serialization of PendingClusterTask.timeInQueue. #8077

Closed

Conversation

jpountz
Copy link
Contributor

@jpountz jpountz commented Oct 14, 2014

This parameter is serialized as a vLong while it could sometimes be negative.

This parameter is serialized as a vLong while it could sometimes be negative.
@s1monw
Copy link
Contributor

s1monw commented Oct 14, 2014

LGTM

@jpountz jpountz closed this in cd8e023 Oct 14, 2014
jpountz added a commit that referenced this pull request Oct 14, 2014
This parameter is serialized as a vLong while it could sometimes be negative.

Close #8077
jpountz added a commit that referenced this pull request Oct 14, 2014
This parameter is serialized as a vLong while it could sometimes be negative.

Close #8077
@s1monw
Copy link
Contributor

s1monw commented Feb 11, 2015

@s1monw
Copy link
Contributor

s1monw commented Feb 11, 2015

@jpountz ping

@bleskes
Copy link
Contributor

bleskes commented Feb 11, 2015

@s1monw we use -1 to signal stuff in the queue which are not updateTasks (as far as I can tell those are timeout notifications). See https://github.com/elasticsearch/elasticsearch/blob/master/src/main/java/org/elasticsearch/cluster/service/InternalClusterService.java#L306

I think this is what is broken. We should have a common queue item base class that gives as the time inserted + a source and only have these in queue (UpdateTask should inherit from it).

@s1monw
Copy link
Contributor

s1monw commented Feb 11, 2015

I just wanna fix the assertion here. We arlready have new code that transfers it correctly. WE can't fix this differently sorry.

@s1monw
Copy link
Contributor

s1monw commented Feb 11, 2015

^^ by that I mean for nodes < 1.4.0

@bleskes
Copy link
Contributor

bleskes commented Feb 11, 2015

My suggestion implies we never write negative values. if we never have to write a negative value, I think the problem is solved for <1.4 as well.

maybe we should make sure the value to vlong is positive?

maybe you meant writing a 0 where we see -1 (which is a valid value now). This is also a workaround but is not the "right" thing. Just wanted to share the idea of a proper fix.

@s1monw
Copy link
Contributor

s1monw commented Feb 11, 2015

again the issue is fixed we use writeLong if you wanna communicate this differently that's all fine with me but it won't fix anything.

@jpountz
Copy link
Contributor Author

jpountz commented Feb 11, 2015

@s1monw are you suggesting we should fix it by removing the assertion?

@s1monw
Copy link
Contributor

s1monw commented Feb 11, 2015

@s1monw are you suggesting we should fix it by removing the assertion?

no I am suggesting to fix the BWC code to send out.writeVLong(Math.max(0, timeInQueue)); since the serialization will be broken if you pass a neg value to writeVLong no?

@jpountz
Copy link
Contributor Author

jpountz commented Feb 11, 2015

Makes sense to me. +1

s1monw added a commit to s1monw/elasticsearch that referenced this pull request 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 elastic#8077
s1monw added a commit that referenced this pull request 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
s1monw added a commit that referenced this pull request 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

Conflicts:
	src/main/java/org/elasticsearch/cluster/service/PendingClusterTask.java
bleskes added a commit to bleskes/elasticsearch that referenced this pull request Feb 12, 2015
…ds that go into InternalClusterService.updateTasksExecutor

 At the moment we sometime submit generic runnables, which make life slightly harder when generated pending task list which have to account for them. This commit adds an abstract TimedPrioritizedRunnable class which should always be used. This class also automatically measures time in queue, which is needed for the pending task reporting.

  Relates to elastic#8077

  Closes elastic#9354
bleskes added a commit to bleskes/elasticsearch that referenced this pull request Feb 12, 2015
…ds that go into InternalClusterService.updateTasksExecutor

 At the moment we sometime submit generic runnables, which make life slightly harder when generated pending task list which have to account for them. This commit adds an abstract TimedPrioritizedRunnable class which should always be used. This class also automatically measures time in queue, which is needed for the pending task reporting.

  Relates to elastic#8077

  Closes elastic#9354
  Closes elastic#9671
bleskes added a commit to bleskes/elasticsearch that referenced this pull request Feb 12, 2015
…ds that go into InternalClusterService.updateTasksExecutor

 At the moment we sometime submit generic runnables, which make life slightly harder when generated pending task list which have to account for them. This commit adds an abstract TimedPrioritizedRunnable class which should always be used. This class also automatically measures time in queue, which is needed for the pending task reporting.

  Relates to elastic#8077

  Closes elastic#9354
  Closes elastic#9671
bleskes added a commit that referenced this pull request Feb 12, 2015
…ds that go into InternalClusterService.updateTasksExecutor

At the moment we sometime submit generic runnables, which make life slightly harder when generated pending task list which have to account for them. This commit adds an abstract TimedPrioritizedRunnable class which should always be used. This class also automatically measures time in queue, which is needed for the pending task reporting.

Relates to #8077

Closes #9354
Closes #9671
@clintongormley clintongormley changed the title Internal: Fix serialization of PendingClusterTask.timeInQueue. Fix serialization of PendingClusterTask.timeInQueue. Jun 7, 2015
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
This parameter is serialized as a vLong while it could sometimes be negative.

Close elastic#8077
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 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 elastic#8077
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
…ds that go into InternalClusterService.updateTasksExecutor

 At the moment we sometime submit generic runnables, which make life slightly harder when generated pending task list which have to account for them. This commit adds an abstract TimedPrioritizedRunnable class which should always be used. This class also automatically measures time in queue, which is needed for the pending task reporting.

  Relates to elastic#8077

  Closes elastic#9354
  Closes elastic#9671
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 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 elastic#8077

Conflicts:
	src/main/java/org/elasticsearch/cluster/service/PendingClusterTask.java
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.

None yet

4 participants