Skip to content

Help relieve some memory pressure on brokers & coordinator#2751

Closed
nishantmonu51 wants to merge 1 commit intoapache:masterfrom
metamx:bard-mem
Closed

Help relieve some memory pressure on brokers & coordinator#2751
nishantmonu51 wants to merge 1 commit intoapache:masterfrom
metamx:bard-mem

Conversation

@nishantmonu51
Copy link
Copy Markdown
Member

Help relieve some memory pressure on the broker and coordinator.
This has a side effect that DataSegment now keeps the dimension/metric names in sorted way.

From Histogram -
6: 18221074 583074368 com.google.common.collect.RegularImmutableList
8: 9138836 511774816 io.druid.timeline.DataSegment
16: 3258048 52128768 io.druid.timeline.partition.NoneShardSpec

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Mar 28, 2016

@nishantmonu51 what is the % improvement?

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.

should be final

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.

I think we can also make Jackson use this by putting @JsonCreator on a method like public static NoneShardSpec instance() { return INSTANCE; }

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

done and added a serde test too.

@nishantmonu51
Copy link
Copy Markdown
Member Author

@fjy for our setup it is expected to relieve it by around 4-5 %.
percentages will depending on how much repetition is there in the list of dimensions and metrics.
It will be significant only for large clusters.

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Mar 31, 2016

👍

Help relieve some memory pressure on brokers & master when it has to
manage large number of segments.

fix tests

review comments
@xvrl
Copy link
Copy Markdown
Member

xvrl commented Apr 4, 2016

Do we expect dimensions in the loadSpec to reflect the order of dimensions used for aggregation? Given that dimension order matters it might be worthwhile to preserve the order defined at aggregation time, anyone else have an opinion about this?

@xvrl
Copy link
Copy Markdown
Member

xvrl commented Apr 4, 2016

@nishantmonu51 how much difference does it make if we don't sort the list, and instead use the dimension order used for indexing?

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Apr 5, 2016

@nishantmonu51 conflicts

@fjy fjy added this to the 0.9.1 milestone Apr 12, 2016
@fjy
Copy link
Copy Markdown
Contributor

fjy commented Apr 12, 2016

👍

@xvrl
Copy link
Copy Markdown
Member

xvrl commented Apr 12, 2016

@fjy my comments haven't been addressed yet

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Apr 12, 2016

@xvrl I didn't merge :P

@nishantmonu51
Copy link
Copy Markdown
Member Author

had a discussion with @xvrl on this one.
Since dimension order in DataSegment represent the order of dimensions in the segment and this PR breaks that order, I am closing this for now.

@nishantmonu51
Copy link
Copy Markdown
Member Author

Also, the changes in #2784 help more in reducing the memory pressure.

@licl2014
Copy link
Copy Markdown
Contributor

licl2014 commented Oct 7, 2018

@nishantmonu51 this PR merged in 0.12.X?,i can't find it in 0.9.1 and 0.10.X

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants