Fixes pre and post offset serialisation for histogram aggs #7313
Conversation
| out.writeVLong(preOffset); | ||
| out.writeVLong(postOffset); | ||
| out.writeLong(preOffset); | ||
| out.writeLong(postOffset); |
There was a problem hiding this comment.
The problem here is that it would break multi-version clusters. We still need to read/write vLong depending on in/out.getVersion so that at least positive offsets work.
|
I was first afraid this bug might have been introduced by #6980 but it seems that it is not the case, right? Just left a comment about backward compatibility of the stream. Otherwise it looks good. |
|
Yep, I think it was present before I made that change and refactored the class. Have updated the serialisation to solve the backwards compatibility issue |
|
I think we shouldn't change the serialization format in a minor release, so I would rather fix it in 1.4.0 only? Otherwise LGTM. |
| } | ||
|
|
||
| @Override | ||
| protected Settings nodeSettings(int nodeOrdinal) { |
There was a problem hiding this comment.
It think this is problematic since this forces LocalTransport all the time even if we run with network tests. I think you should add a setting to AssertingLocalTransport that allows you to set the min version? then you can just pass the min version here instead of the TransportModule.TRANSPORT_TYPE_KEY I hope that makes sense
|
LGTM thanks for all the iterations! |
Changes the serialisation of pre and post offset to use Long instead of VLong so that negative values are supported. This actually only showed up in the case where minDocCount=0 as the rounding is only serialised in this case. Closes #7312
clintongormley
changed the title
Changes the serialisation of pre and post offset to use Long instead of VLong so that negative values are supported. This actually only showed up in the case where minDocCount=0 as the rounding is only serialised in this case.
Closes #7312