Flush oldest sink when total rows in all sinks exceeds maxRowsInMemory

[navis]

Currently maxRowsInMemory in RealtimeIndexTask is applied per sink base, which can cause OOM if too many sinks under the size of the threshold are created for some reason. This patch periodically checks total count of currently active sinks and persist the least recently accessed one if it exceeds the threshold.

[nishantmonu51]

its already volatile, do we really need to take a swaplock here ?

[navis]

If index can be null, it's not safe to do something like return index != null && index.canAppendRow(). I thought the overhead is minimal. But volatile needed to be removed then.

[fjy]

@navis how did you create so many sinks at the same time without triggering maxPendingPersists to begin throttling ingestion?

[navis]

@fjy I've used a day log file in local filesystem with hour segment granularity. Any of row count for an hour didn't exceeded maxRowsInMemory and persisting didn't happened. Consequently index task throws OOM after passing half of the day(13~14 sinks, I remember).

I know it's not general use cases and we can pass it with lower maxRowsInMemory by speculation. But this small patch can lessen the pain.

[fjy]

@gianm do you mind taking a look?

[jon-wei]

Maybe this is unnecessary/unrealistic, but should this be determined based on the configured maxRowsInMemory? i.e., if the user configures maxRowsInMemory < 100000, they could in theory run into the same OOM issue.

Perhaps this isn't a practical problem though unless the per-row size is colossal and/or available memory is tiny.

[navis]

Changed to Math.min(config.getMaxRowsInMemory() >> 2, MAX_ROW_EXCEED_CHECK_COUNT). Thanks.

[fjy]

@gianm

[gianm]

This file doesn't need to change

[navis]

reverted

[gianm]

Floats.BYTES

[navis]

good

[gianm]

@navis, this looks like a great change that would make druid easier to configure. my main comments are #2459 (comment) & #2459 (comment).

[navis]

@gianm Because currently we are not using any kind of realtime ingestion (it's very very bad part of our on-going projects) and don't have a chance to apply this, it seemed not good idea for me to make patchs for this kind of issues. I'm good to this to be assigned by someone or to be closed with 'later' state.

[fjy]

I'm removing the milestone for now based on author's comments.

[stale]

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@druid.apache.org list. Thank you for your contributions.

[stale]

This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.