Allow to serialize negative thread pool sizes #6486
Conversation
|
Note: There are several solutions to this problem. My first approach was to allow -1 in Also, the bwc layer could potentially be removed for master. Maybe someone has a better idea |
| @Test(expected = AssertionError.class) | ||
| public void testThatNegativeQueueSizesThrowExceptionInOldVersions() throws Exception { | ||
| ThreadPool.Info info = new ThreadPool.Info("foo", "search", 1, 10, TimeValue.timeValueMillis(3000), SizeValue.parseSizeValue("-1")); | ||
| output.setVersion(Version.V_1_2_1); |
There was a problem hiding this comment.
maybe randomize the version a bit here?
|
@spinscale can you pick this up any time soon or should somebody else pick it up? |
|
@s1monw updated the PR, feel free to take another look. |
| @@ -173,7 +177,9 @@ public static SizeValue parseSizeValue(String sValue, SizeValue defaultValue) th | |||
| } | |||
| long singles; | |||
| try { | |||
| if (sValue.endsWith("b")) { | |||
| if (sValue.equals("-1")) { | |||
There was a problem hiding this comment.
IMO we should not handle this in here... we should special case this in ThreadPool.java and maybe just use null as the value for the queue or move the UNBOUNDED sentinel there.
|
I left comments on the latest push |
|
added the |
| out.writeBoolean(isQueueSizeBounded); | ||
| if (isQueueSizeBounded) { | ||
| queueSize.writeTo(out); | ||
| } else { |
There was a problem hiding this comment.
this else clause is unnecessary or infact wrong :) no? we should not write UNBOUNDED
|
ok so now I am a bit lost... I don't get why we add unittests for SizeValue with negative values? didn't we fix this before to make sure you can't pass a negative value? |
|
oh boy, thats what happens when you push things before hopping on a plane after giving a workshop all day, sorry for that! back to throwing an exception and and incorporated your review comment |
|
LGTM |
As a SizeValue is used for serializing the thread pool size, a negative number resulted in throwing an exception when deserializing (using -ea an assertionerror was thrown). This fixes a check for changing the serialization logic, so that negative numbers are read correctly, by adding an internal UNBOUNDED value. Closes elastic#6325 Closes elastic#5357
…ization This fixes an issue introduced by the serialization changes in #6486 which are not needed at all. Node that the serialization itself is not broken but the TransportClient uses its own version on initial connect and getting the NodeInfos.
…ization This fixes an issue introduced by the serialization changes in #6486 which are not needed at all. Node that the serialization itself is not broken but the TransportClient uses its own version on initial connect and getting the NodeInfos.
…ization This fixes an issue introduced by the serialization changes in #6486 which are not needed at all. Node that the serialization itself is not broken but the TransportClient uses its own version on initial connect and getting the NodeInfos.
…ization This fixes an issue introduced by the serialization changes in #6486 which are not needed at all. Node that the serialization itself is not broken but the TransportClient uses its own version on initial connect and getting the NodeInfos.
clintongormley
changed the title
clintongormley
changed the title
…ization This fixes an issue introduced by the serialization changes in elastic#6486 which are not needed at all. Node that the serialization itself is not broken but the TransportClient uses its own version on initial connect and getting the NodeInfos.
…ization This fixes an issue introduced by the serialization changes in elastic#6486 which are not needed at all. Node that the serialization itself is not broken but the TransportClient uses its own version on initial connect and getting the NodeInfos.
As a SizeValue is used for serializing the thread pool size, a negative number
resulted in throwing an exception when deserializing (using -ea an assertionerror
was thrown).
This fixes a check for changing the serialization logic, so that negative numbers are read correctly.
Closes #6325
Closes #5357