Skip to content

Fix coordinator's dataSource api with full parameter#5662

Merged
gianm merged 5 commits intoapache:masterfrom
jihoonson:fix-datasource-full
Apr 20, 2018
Merged

Fix coordinator's dataSource api with full parameter#5662
gianm merged 5 commits intoapache:masterfrom
jihoonson:fix-datasource-full

Conversation

@jihoonson
Copy link
Copy Markdown
Contributor

@jihoonson jihoonson commented Apr 18, 2018

Fixes #5661.


This change is Reviewable

@jihoonson jihoonson added the Bug label Apr 18, 2018
@jihoonson jihoonson added this to the 0.12.1 milestone Apr 18, 2018
@gianm
Copy link
Copy Markdown
Contributor

gianm commented Apr 19, 2018

@jihoonson - does the returned JSON match what 0.11.0 & earlier returned? If so, this patch looks good to me.

@jihoonson
Copy link
Copy Markdown
Contributor Author

@gianm good point. Fixed the result format.


if (full != null) {
return Response.ok(dataSource).build();
final Map<String, Object> fullResult = ImmutableMap.of(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this, how about having @JsonProperty on the getSegments() method, and having the @JsonCreator constructor accept segments and make the map out of it? It should be equivalent but cleaner -- usually we try to handle serde through annotations.

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Apr 19, 2018

@jihoonson this Travis failure seems related,

  DatasourcesResourceTest.testFullGetTheDataSource:328 ClassCast com.google.comm...

I think it would be fixed if you used the suggestion from #5662 (comment).

@jihoonson
Copy link
Copy Markdown
Contributor Author

@gianm thanks. Addressed your comment. I also fixed the result order - it was previously always sorted.

Copy link
Copy Markdown
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thank you!

ImmutableMap.of(),
ImmutableMap.copyOf(segmentMap)
);
return new ImmutableDruidDataSource(dataSourceName, Collections.emptyMap(), segmentMap);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, I think: if we're building the segmentMap from scratch here anyway, might as well start it off as an ImmutableSortedMap. That way ImmutableSortedMap.copyOfSorted is a no-op and we avoid the extra copy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This causes the below error because the same segments can be added to the segmentMap multiple times. Reverted this change.

java.lang.IllegalArgumentException: Multiple entries with same key: datasource1_2010-01-22T00:00:00.000Z_2010-01-23T00:00:00.000Z_null=DataSegment{size=20, shardSpec=NoneShardSpec, metrics=[], dimensions=[], version='null', loadSpec=null, interval=2010-01-22T00:00:00.000Z/2010-01-23T00:00:00.000Z, dataSource='datasource1', binaryVersion='9'} and datasource1_2010-01-22T00:00:00.000Z_2010-01-23T00:00:00.000Z_null=DataSegment{size=20, shardSpec=NoneShardSpec, metrics=[], dimensions=[], version='null', loadSpec=null, interval=2010-01-22T00:00:00.000Z/2010-01-23T00:00:00.000Z, dataSource='datasource1', binaryVersion='9'}
	at io.druid.server.http.DatasourcesResourceTest.testSimpleGetTheDataSourceManyTiers(DatasourcesResourceTest.java:404)

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Apr 20, 2018

Thanks - merging!

@gianm gianm merged commit ca3f833 into apache:master Apr 20, 2018
sathishsri88 pushed a commit to sathishs/druid that referenced this pull request May 8, 2018
* Fix coordinator's dataSource api with full parameter

* address comment

* Add a constructor for json serde and fix result order

* Change to immutableSortedMap

* Revert immutableSortedMap to treeMap
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants