Skip to content

Remove "granularity" from IngestSegmentFirehose.#4110

Merged
fjy merged 1 commit intoapache:masterfrom
gianm:isff
Mar 24, 2017
Merged

Remove "granularity" from IngestSegmentFirehose.#4110
fjy merged 1 commit intoapache:masterfrom
gianm:isff

Conversation

@gianm
Copy link
Copy Markdown
Contributor

@gianm gianm commented Mar 24, 2017

It wasn't doing anything useful (the sequences were being concatted, and
cursor.getTime() wasn't being called) and it defaulted to Granularities.NONE.
Changing it to Granularities.ALL gave me a 700x+ performance boost on a
small dataset I was reindexing (2m27s to 365ms). Most of that was from avoiding
making a lot of unnecessary column selectors.

@gianm
Remove "granularity" from IngestSegmentFirehose.
0e47c9e 
It wasn't doing anything useful (the sequences were being concatted, and
cursor.getTime() wasn't being called) and it defaulted to Granularities.NONE.
Changing it to Granularities.ALL gave me a 700x+ performance boost on a
small dataset I was reindexing (2m27s to 365ms). Most of that was from avoiding
making a lot of unnecessary column selectors.
@gianm gianm added this to the 0.10.1 milestone Mar 24, 2017
leventov
leventov reviewed Mar 24, 2017
View reviewed changes
Comment thread server/src/main/java/io/druid/segment/realtime/firehose/IngestSegmentFirehose.java
@@ -77,7 +76,7 @@ public Sequence<InputRow> apply(WindowedStorageAdapter adapter)
Filters.toFilter(dimFilter),
Copy link
Copy Markdown
Member

@leventov leventov Mar 24, 2017

Choose a reason for hiding this comment

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

Maybe could be further simplified by not calling concat() a few lines above.

Copy link
Copy Markdown
Contributor Author

@gianm gianm Mar 24, 2017

Choose a reason for hiding this comment

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

How else would this be turned into a Sequence<InputRow> rather than Sequence<Sequence<InputRow>>?

Copy link
Copy Markdown
Member

@leventov leventov Mar 24, 2017

Choose a reason for hiding this comment

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

Ok, seems there is no way.

@fjy fjy merged commit b4289c0 into apache:master Mar 24, 2017
gianm added a commit to implydata/druid-public that referenced this pull request May 6, 2017
@gianm
Remove "granularity" from IngestSegmentFirehose. (apache#4110)
19b0f5e 
It wasn't doing anything useful (the sequences were being concatted, and
cursor.getTime() wasn't being called) and it defaulted to Granularities.NONE.
Changing it to Granularities.ALL gave me a 700x+ performance boost on a
small dataset I was reindexing (2m27s to 365ms). Most of that was from avoiding
making a lot of unnecessary column selectors.
gianm added a commit to implydata/druid-public that referenced this pull request May 16, 2017
@gianm
Remove "granularity" from IngestSegmentFirehose. (apache#4110)
02e4220 
It wasn't doing anything useful (the sequences were being concatted, and
cursor.getTime() wasn't being called) and it defaulted to Granularities.NONE.
Changing it to Granularities.ALL gave me a 700x+ performance boost on a
small dataset I was reindexing (2m27s to 365ms). Most of that was from avoiding
making a lot of unnecessary column selectors.
@jon-wei jon-wei mentioned this pull request Jun 13, 2017
Closed
@himanshug
Copy link
Copy Markdown
Contributor

himanshug commented Oct 31, 2017

@gianm it appears that this breaks re-indexing which expects IngestSegmentFireHose to give individual rows from the segment without any truncation.
how about making ALL default granularity but still providing the option for caller to change the granularity so that re-indexing path can stay same ?
I wonder if this impacts re-indexing done via local index task too.

@gianm gianm deleted the isff branch October 31, 2017 18:04
@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Oct 31, 2017

@himanshug What specifically has broken? IIRC the rows still do have their original, unchanged timestamps -- the only difference is that the cursor timestamps are truncated. But reindexing shouldn't be using the cursor timestamps anyway.

@himanshug
Copy link
Copy Markdown
Contributor

himanshug commented Oct 31, 2017

ok, I assumed CursorFactory.makeCursor(..) could truncate timestamps based on provided granularity. its fine if it returns rows without truncation. thanks.

@gianm
Copy link
Copy Markdown
Contributor Author

gianm commented Oct 31, 2017

IIRC what happens is the cursor.getTime() is truncated, but the timestamp on the actual rows (row.getTimestampFromEpoch()) is not truncated, since it comes from the time column, not from the cursor. So I think it should be ok. If you notice anything different please raise it…

leventov
leventov reviewed Mar 2, 2018
View reviewed changes
Comment thread indexing-hadoop/src/main/java/io/druid/indexer/hadoop/DatasourceIngestionSpec.java
@@ -171,7 +158,6 @@ public DatasourceIngestionSpec withQueryGranularity(Granularity granularity)
intervals,
Copy link
Copy Markdown
Member

@leventov leventov Mar 2, 2018

Choose a reason for hiding this comment

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

Now this method is effectively just "clone", the method argument is unused.

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

Reviewers

@leventov leventov leventov approved these changes

Assignees

No one assigned

Labels

Performance

Projects

None yet

Milestone

0.10.1

Development

Successfully merging this pull request may close these issues.

4 participants

@gianm @himanshug @leventov @fjy