Scavenge index

[lscpike]

This is based on PR #1637.

The index currently gets scavenged during a level merge. This can happen at any time and can impact performance of the node. It is also very inefficient as each level performs a new scavenge even though it may have just been scavenged for a lower level. If the database hasn't been scavenged since the last merge this is a complete waste of time.

This PR adds an explicit scavenge operation to the TableIndex. This is called at the end of a DB scavenge meaning there is definite benefit to doing the work. It also means it can be stopped during high load situations (thanks to PR #1632.)

Notes

• The index scavenge blocks the awaiting tables queue. This shouldn't be a problem as the impact is no different to a normal merge.
• The scavenge code is based upon the merge code.
• There's a decent test suite in the PR.
• The merge still has a lot of the scavenge code in there for the v1 upgrade path. If v1 support is dropped in future this code gets a lot smaller.
• The activity is logged to the $scavenges stream to fit the general pattern.

[lscpike]

Just to note this my last PR around scavenging.

[gregoryyoung]

Can you squash the changes?

[lscpike]

Sure. You want the whole lot as 1 commit? There's a lot of files touched across the 7 prs. Each PR is mergable/working if you do them in order. The diffs will obviously get smaller each time.

…

On Fri, 25 May 2018, 05:45 Greg Young, ***@***.***> wrote: Can you squash the changes? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1638 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGr263vFAvLVd2WZPMPbaVIDiV91_l1Qks5t14xcgaJpZM4UMp6Z> .

[gregoryyoung]

I meant just for this one (in case we later need to revert). Its far easier to have the commit history with one commit for the feature.

[lscpike]

Makes sense! Want the same for the others; faster scavenge and scavenge log clean up? I need to rebase anyway.

[lscpike]

We realized that the OptimizeIndexMerge flag results in a lot of memory sitting unused when the index scavenge is complete. As this optimisation now only applies when doing a scavenge it makes sense to free the memory at the end of the scavenge process.

I can squash in if you want, but figured it's easier to see what we've changed as is for now.

[megakid]

Any movement on this or if not, timelines for progression?

[riccardone]

@lscpike @megakid we like the stop operation that is implemented in your PR. Thanks for that.
We are not sure about the changes that you have done with multi-threading. The scavenge operation as is today is a very simple operation and we would like to keep it simple in order to serve all the different scenarios and environments where our clients are running ES.
Could it be possible keep as is or with minimal modification the current implementation and provide a separate implementation with the parallel/multithread execution?
Something to be driven by command line param (--parallel-scavenge) or http?

[lscpike]

By default it is single threaded. The thread count is exposed as a http option. This feature is very important for us as a 3 week scavenge time is unmanagable where a 4 day scavenge time is more reasonable (real world example).

…

On Fri, 27 Jul 2018, 10:06 Riccardo Di Nuzzo, ***@***.***> wrote: @lscpike <https://github.com/lscpike> @megakid <https://github.com/megakid> we like the stop operation that is implemented in your PR. Thanks for that. We are not sure about the changes that you have done with multi-threading. The scavenge operation as is today is a very simple operation and we would like to keep it simple in order to serve all the different scenarios and environments where our clients are running ES. Could it be possible keep as is or with minimal modification the current implementation and provide a separate implementation with the parallel/multithread execution? Something to be driven by command line param (--parallel-scavenge) or http? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1638 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGr26_Cta-O69MM18TmjUmtRMwOTZHohks5uKtgBgaJpZM4UMp6Z> .

[riccardone]

I get the fact that there is a thread param count. I was asking about the possibility to keep separate your new scavenge multi-threaded implementation from the existing and use your one on demand. That's the reason for the --parallel-scavenge suggestion. An alternative to the command line param, the presence of the http thread count option could be used to decide to use your new implementation or the original.

[lscpike]

The parallel implementation is only about 3 lines of code. Are you more concerned about the performance optimizations we've made?

Would it be possible to start merging the prior PRs to make the faster scavenge and index scavenge PRs easier to review? I'll rebase the others as each PR is merged to give a clean diff. We can then focus on getting the scavenge optimizations as low risk as possible for you.

[gregoryyoung]

The parallel can work out to be slower was more the comment due to disk head thrashing vs sequential reading

[lscpike]

The default thread count of 1 will result in sequential reads of the chunks. It's then up to the user not to use the hidden option (not in the UI) that might have a decrease in performance for their disks. That can be in the documentation if required.

[lscpike]

We're running this in production as a custom build now. Thought you might be interested to see the scavenge log.

[shaan1337]

@lscpike @megakid Kindly also rebase these commits (including 6b18816)

[lscpike]

Rebased as requested.

[riccardone]

@lscpike I'm doing the final tests of this PR.
I wonder if it could be possible to log the 'total space saved' as it was before the changes?
https://github.com/lscpike/EventStore/blob/feature/scavenge_index/src/EventStore.Core/TransactionLog/Chunks/TFChunkScavenger.cs#L204
example:
SCAVENGING: total time taken: 00:08:47.3769352, total space saved: 802301658

[riccardone]

@lscpike in PTableConstruction can I ask why the new method is called Scavenged and not Scavenge?

[lscpike]

Probably a typo. I'll fix in a bit.

…

On Mon, 12 Nov 2018, 12:01 Riccardo Di Nuzzo ***@***.*** wrote: @lscpike <https://github.com/lscpike> in PTableConstruction can I ask why the new method is called Scavenged and not Scavenge? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1638 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGr26_5KdhIjb5emHp0dyqdXn4_fquAuks5uuWMngaJpZM4UMp6Z> .

[lscpike]

Just went to fix it and realised it's a factory method. It's createing a "Scavenged" PTable. I'll leave as is unless you have a different opinion.

[riccardone]

I tested these changes with small, medium and large databases and I confirm that the code works as expected. Well done, this is a very good improvement. Thanks.

[riccardone]

I tested these changes with small, medium and large databases and I confirm that the code works as expected. Well done, this is a very good improvement. Thanks.

[riccardone]

@lscpike it's probably better if you rebase your PR (not me). Now fw is on 4.7.1 and there is only a simple conflict to fix on index_map. After that I will proceed merging the Faster_scavenge and this PR into master. Thanks

[lscpike]

I rebased and resolved this. This branch includes the faster_scavenge changes. Do you want me to rebase that PR too or are you happy to merge all from here?

[riccardone]

I'm happy to merge all from here, thanks.

[riccardone]

I tested these changes with small, medium and large databases and I confirm that the code works as expected. Well done, this is a very good improvement. Thanks.

[ChrisChinchilla]

Docs noted @riccardone and @shaan1337