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

Fixes pre and post offset serialisation for histogram aggs #7313

Merged
merged 1 commit into from Aug 21, 2014

Conversation

Projects
None yet
4 participants
@colings86
Copy link
Member

colings86 commented Aug 18, 2014

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

@colings86 colings86 added the review label Aug 18, 2014

@jpountz

View changes

src/main/java/org/elasticsearch/common/rounding/Rounding.java Outdated
out.writeVLong(preOffset);
out.writeVLong(postOffset);
out.writeLong(preOffset);
out.writeLong(postOffset);

This comment has been minimized.

Copy link
@jpountz

jpountz Aug 18, 2014

Contributor

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.

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 18, 2014

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.

@jpountz jpountz removed the review label Aug 18, 2014

@colings86

This comment has been minimized.

Copy link
Member Author

colings86 commented Aug 18, 2014

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

@jpountz

This comment has been minimized.

Copy link
Contributor

jpountz commented Aug 19, 2014

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.

@jpountz jpountz removed the review label Aug 19, 2014

@colings86 colings86 self-assigned this Aug 19, 2014

@colings86 colings86 added the review label Aug 21, 2014

}

@Override
protected Settings nodeSettings(int nodeOrdinal) {

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 21, 2014

Contributor

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

@s1monw

View changes

src/test/java/org/elasticsearch/test/transport/AssertingLocalTransport.java Outdated

@Inject
public AssertingLocalTransport(Settings settings, ThreadPool threadPool, Version version) {
super(settings, threadPool, version);
final long seed = settings.getAsLong(ElasticsearchIntegrationTest.SETTING_INDEX_SEED, 0l);
random = new Random(seed);
minVersion = Version.fromString(settings.get(ASSERTING_TRANSPORT_MIN_VERSION_KEY, null));

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 21, 2014

Contributor

can't you just use settings.getAsVersion here? and on the other end you just use builder.put(ASSERTING_TRANSPORT_MIN_VERSION_KEY, version)

@s1monw

View changes

src/test/java/org/elasticsearch/test/ElasticsearchTestCase.java Outdated
@@ -311,6 +307,30 @@ public static Version randomVersion(Random random) {
return SORTED_VERSIONS.get(random.nextInt(SORTED_VERSIONS.size()));
}

public static Version randomVersionBetween(Version minVersion, Version maxVersion) {

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 21, 2014

Contributor

would you mind documenting these two methods?

@s1monw

View changes

src/test/java/org/elasticsearch/search/aggregations/bucket/DateHistogramOffsetTests.java Outdated
import static org.hamcrest.Matchers.equalTo;

/**
*

This comment has been minimized.

Copy link
@s1monw

s1monw Aug 21, 2014

Contributor

add a quick doc string why this is a sep. class?

@s1monw

This comment has been minimized.

Copy link
Contributor

s1monw commented Aug 21, 2014

LGTM thanks for all the iterations!

@colings86 colings86 force-pushed the colings86:fix/7312 branch Aug 21, 2014

Aggregations: Fixes pre and post offset serialisation for histogram aggs
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

@colings86 colings86 force-pushed the colings86:fix/7312 branch to 8550b9e Aug 21, 2014

@colings86 colings86 merged commit 8550b9e into elastic:master Aug 21, 2014

@colings86 colings86 assigned colings86 and unassigned colings86 Aug 21, 2014

@colings86 colings86 deleted the colings86:fix/7312 branch Aug 21, 2014

@jpountz jpountz removed the review label Aug 26, 2014

@clintongormley clintongormley changed the title Aggregations: Fixes pre and post offset serialisation for histogram aggs Fixes pre and post offset serialisation for histogram aggs Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.