Skip to content

MB-60971: Avoiding unnecessary persister notifs from merger#2003

Merged
abhinavdangeti merged 3 commits intomasterfrom
noOpMerge
Mar 26, 2024
Merged

MB-60971: Avoiding unnecessary persister notifs from merger#2003
abhinavdangeti merged 3 commits intomasterfrom
noOpMerge

Conversation

@Thejas-bhat
Copy link
Copy Markdown
Member

@Thejas-bhat Thejas-bhat commented Mar 25, 2024

  • There are chances that the merger doesn't see any eligible segments to be merged in the current iteration which causes the tasks list to be empty. In this situation, merger which didn't update the root snapshot would notify the persister.
  • Now, if the persister was napping at this point of time, and assuming there were mutations coming into the system (so the root snapshot would be updated by the introducer) would lead to the persister to be awoken and start flushing out the in-memory segments to disk.
  • Perhaps a better behaviour would be to not let the merger notify the persister when there is no change in the root snapshot. This would help the persister to perform healthier in memory merging of the segments before persisting out to disk thereby helping in controlling the number of IO ops scorch would do.

Some numbers on local testing (~4.18M dataset with lorem ipsum content)

With patch

  "num_bytes_used_ram": 186097560,
  "test_bucket:test_bucket._default.test:num_file_merge_ops": 7,
  "test_bucket:test_bucket._default.test:num_mutations_to_index": 0,
  "test_bucket:test_bucket._default.test:num_persister_nap_merger_break": 4,
  "test_bucket:test_bucket._default.test:num_persister_nap_pause_completed": 80,

Without patch

  "num_bytes_used_ram": 265234328,
  "test_bucket:test_bucket._default.test:num_file_merge_ops": 10,
  "test_bucket:test_bucket._default.test:num_mutations_to_index": 0,
  "test_bucket:test_bucket._default.test:num_persister_nap_merger_break": 69,
  "test_bucket:test_bucket._default.test:num_persister_nap_pause_completed": 45,

abhinavdangeti
abhinavdangeti previously approved these changes Mar 25, 2024
@abhinavdangeti abhinavdangeti changed the title Avoiding unnecessary persister notifs from merger MB-60971: Avoiding unnecessary persister notifs from merger Mar 26, 2024
@abhinavdangeti abhinavdangeti merged commit 4fd8313 into master Mar 26, 2024
@abhinavdangeti abhinavdangeti deleted the noOpMerge branch March 26, 2024 16:39
@abhinavdangeti abhinavdangeti restored the noOpMerge branch March 27, 2024 00:20
@abhinavdangeti
Copy link
Copy Markdown
Member

This commit has been reverted with #2005

abhinavdangeti added a commit that referenced this pull request Apr 4, 2024
…#2006)

Authored-by: @Thejas-bhat 
Original: #2003

- There are chances that the merger doesn't see any eligible segments to
be merged in the current iteration which causes the tasks list to be
empty. In this situation, merger which didn't update the root snapshot
would notify the persister.
- Now, if the persister was napping at this point of time, and assuming
there were mutations coming into the system (so the root snapshot would
be updated by the introducer) would lead to the persister to be awoken
and start flushing out the in-memory segments to disk.
- Perhaps a better behaviour would be to let the persister nap for the
remaining
duration such and then let the persister do some work. This would also
help in merger
waiting for the notification reply (which is like an interrupt fashion
type of wait)
rather than doing something more expensive of letting merger continue to
do work
(which the earlier commits of this PR was doing). 

Some numbers on local testing (~4.18M dataset with lorem ipsum content)

With patch
```
"num_bytes_used_ram": 224100280,
"num_files_on_disk": 34,
"test_bucket:test_bucket._default.travel:num_file_merge_ops": 6,
"test_bucket:test_bucket._default.travel:num_file_merge_plan": 0,
"test_bucket:test_bucket._default.travel:num_files_on_disk": 34,
"test_bucket:test_bucket._default.travel:num_mem_merge_ops": 70,
"test_bucket:test_bucket._default.travel:num_persister_nap_merger_break": 4,
"test_bucket:test_bucket._default.travel:num_persister_nap_pause_completed": 69,
"test_bucket:test_bucket._default.travel:num_root_filesegments": 16,
"test_bucket:test_bucket._default.travel:num_root_memorysegments": 0,

"TotFileMergePlan": 45,
"TotFileMergePlanErr": 0,
"TotFileMergePlanNone": 1,
"TotFileMergePlanOk": 44,
```

Without patch
```
"num_bytes_used_ram": 252726152,
"num_files_on_disk": 45,
"test_bucket:test_bucket._default.travel:num_file_merge_ops": 11,
"test_bucket:test_bucket._default.travel:num_file_merge_plan": 0,
"test_bucket:test_bucket._default.travel:num_files_on_disk": 45,
"test_bucket:test_bucket._default.travel:num_mem_merge_ops": 129,
"test_bucket:test_bucket._default.travel:num_persister_nap_merger_break": 85,
"test_bucket:test_bucket._default.travel:num_persister_nap_pause_completed": 44,
"test_bucket:test_bucket._default.travel:num_root_filesegments": 33,
"test_bucket:test_bucket._default.travel:num_root_memorysegments": 0,

"TotFileMergePlan": 96,
"TotFileMergePlanErr": 0,
"TotFileMergePlanNone": 1,
"TotFileMergePlanOk": 95,

```

---------

Co-authored-by: Thejas-bhat <thejas.orkombu@couchbase.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants