Support negative numbers in readVLong#22314
Conversation
|
Relates to #22285 |
|
why don't we just make the assertion a hard check as well on the writer end? |
|
I wonder if we should replace the assert with a proper exception instead? Would it have helped to catch this earlier if we had an exception instead? |
How about I throw an exception on the write but still preserve the ability to read negative vlongs so we don't get poisoned by any versions of Elasticsearch that don't have the fix? |
|
I pushed a change that makes the check on writes hard and drops the check on reads. That way if nodes with this code communicate with nodes without this code they'll be able to read the negative numbers. |
There was a problem hiding this comment.
maybe we should also have one of those fun tests that read from a previous version to verify the interaction with previous versions?
There was a problem hiding this comment.
I think it is enough to use the writeVLongNoCheck version to put a negative number on the stream. At least, that was my intent.
There was a problem hiding this comment.
ok that should repro the previous behaviour without assertions enabled you mean? I dunno, given that we are touching how we read and write stuff, I would be happy to have proper bw comp unit tests, especially around negative numbers that we don't cover anywhere in our test infra I guess.
There was a problem hiding this comment.
Makes sense to me. Let me have a look at doing that.
|
I pushed a commit that adds an assertion against some base 64 encoded bytes that'd be generated by 5.1.1 and below so we can be sure we can read from that version. |
jasontedor
left a comment
There was a problem hiding this comment.
Looks good, I left two comments.
There was a problem hiding this comment.
Comment: one and nine -> one and ten
There was a problem hiding this comment.
I don't think the & is needed here, or is that just for symmetry?
There was a problem hiding this comment.
Mostly symmetry, but I can remove it, yeah.
There was a problem hiding this comment.
Oh, I'm fine with symmetry, I just wanted to make sure that I was reading it correctly.
|
I don't think there will be a 5.0.3 (and if there is, I don't think this should go into 5.0.3 otherwise it's too hard to reason about it being fixed in 5.0.3 but not 5.1.1). |
We don't *want* to use negative numbers with `writeVLong` so we have assertions to fail if that happens. On the other hand unforeseen bugs might cause us to write negative numbers so this fixes `readVLong` so that instead of reading a wrong value and corrupting the stream it reads the negative value.
28cd6d0 to
d84a2f9
Compare
* master: (22 commits) Support negative numbers in writeVLong (elastic#22314) UnicastZenPing's PingingRound should prevent opening connections after being closed Add task to clean idea build directory. Make cleanIdea task invoke it. add trace logging to UnicastZenPingTests.testResolveReuseExistingNodeConnections Adds ingest processor headers to exception for unknown processor. (elastic#22315) Remove much ceremony from parsing client yaml test suites (elastic#22311) Support numeric bounds with decimal parts for long/integer/short/byte datatypes (elastic#21972) inner hits: Don't inline inner hits if the query the inner hits is inlined into can't resolve mappings and ignore_unmapped has been set to true Fix stackoverflow error on InternalNumericMetricAggregation Date detection should not rely on a hardcoded set of characters. (elastic#22171) `value_type` is useful regardless of scripting. (elastic#22160) Improve concurrency of ShardCoreKeyMap. (elastic#22316) fixed jdocs and removed already fixed norelease Adds abstract test classes for serialisation (elastic#22281) Introduce translog no-op Provide helpful error message if a plugin exists Clear static variable after suite Repeated language analyzers (elastic#22240) Restore deprecation warning for invalid match_mapping_type values (elastic#22304) Make `-0` compare less than `+0` consistently. (elastic#22173) ...
We don't *want* to use negative numbers with `writeVLong` so throw an exception when we try. On the other hand unforeseen bugs might cause us to write negative numbers (some versions of Elasticsearch don't have the exception, only an assertion) so this fixes `readVLong` so that instead of reading a wrong value and corrupting the stream it reads the negative value.
We don't *want* to use negative numbers with `writeVLong` so throw an exception when we try. On the other hand unforeseen bugs might cause us to write negative numbers (some versions of Elasticsearch don't have the exception, only an assertion) so this fixes `readVLong` so that instead of reading a wrong value and corrupting the stream it reads the negative value.
We don't want to use negative numbers with
writeVLongso throw an exception when we try. On the other
hand unforeseen bugs might cause us to write negative numbers (some versions of Elasticsearch don't have the exception, only an assertion)
so this fixes
readVLongso that instead of reading a wrongvalue and corrupting the stream it reads the negative value.