move remonitor code into DOWN message #3144
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tonysun83
force-pushed
the
re-monitor-compaction-pid
branch
from
1492947
to
727accf
Compare
davisp
requested changes
Sep 10, 2020
src/smoosh/src/smoosh_channel.erl
Outdated
| end; | ||
| Ref = erlang:monitor(process, DbPid), | ||
| DbPid ! {'$gen_call', {self(), Ref}, start_compact}, | ||
| State#state{starting=[{Ref, couch_db:name(Db)}|State#state.starting]}; |
There was a problem hiding this comment.
I don't think this is correct. A compaction could be running due to manual intervention or perhaps if smoosh crashed and left a compaction running. I'd just change the comment to be something like "Compaction is already running, so monitor existing compaction pid".
There was a problem hiding this comment.
there isn't a comment here. are you talking about line 295 :
couch_log:notice("Db ~s continuing compaction",
[smoosh_utils:stringify(DbName)])
to
couch_log:notice("Compaction is already running for ~p, so monitor existing compaction pid ~p",
[smoosh_utils:stringify(DbName), CPID])
There was a problem hiding this comment.
I changed this below
There was a problem hiding this comment.
also, this section is a revert to what we originally had: https://github.com/cloudant/smoosh/pull/54/files#diff-7ff50b91998e5bf2a1f4cf4a8250f607L236-L238
There was a problem hiding this comment.
No, I'm saying the comment on lines 284/285 from before the patch are a comment about "database still compaction...". I think it'd be better to just change that comment rather than changing the whole thing back.
There was a problem hiding this comment.
oh I see what you mean, basically leave the initial check in as well
Smoosh monitors the compactor pid to determine when the compaction jobs finishes, and uses this for its idea of concurrency. However, this isn't accurate in the case where the compaction job has to re-spawn to catch up on intervening changes since the same logical compaction job continues with another pid and smoosh is not aware. In such cases, a smoosh channel with concurrency one can start arbitrarily many additional database compaction jobs. To solve this problem, we added a check to see if a compaction PID exists for a db in `start_compact`. But wee need to add another check because this check is only for shard that comes off the queue. So the following can still occur: 1. Enqueue a bunch of stuff into channel with concurrency 1 2. Begin highest priority job, Shard1, in channel 3. Compaction finishes, discovers compaction file is behind main file 4. Smoosh-monitored PID for Shard1 exits, a new one starts to finish the job 5. Smoosh receives the 'DOWN' message, begins the next highest priority job, Shard2 6. Channel concurrency is now 2, not 1 This change adds another check into the 'DOWN' message so that it checks for that specific shard. If the compaction PID exists then it means a new process was spawned and we just monitor that one and add it back to the queue. The length of the queue does not change and therefore we won’t spawn new compaction jobs.
tonysun83
force-pushed
the
re-monitor-compaction-pid
branch
from
188af27
to
1952852
Compare
jiangphcn
pushed a commit
that referenced
this pull request
Nov 6, 2020
Smoosh monitors the compactor pid to determine when the compaction jobs finishes, and uses this for its idea of concurrency. However, this isn't accurate in the case where the compaction job has to re-spawn to catch up on intervening changes since the same logical compaction job continues with another pid and smoosh is not aware. In such cases, a smoosh channel with concurrency one can start arbitrarily many additional database compaction jobs. To solve this problem, we added a check to see if a compaction PID exists for a db in `start_compact`. But wee need to add another check because this check is only for shard that comes off the queue. So the following can still occur: 1. Enqueue a bunch of stuff into channel with concurrency 1 2. Begin highest priority job, Shard1, in channel 3. Compaction finishes, discovers compaction file is behind main file 4. Smoosh-monitored PID for Shard1 exits, a new one starts to finish the job 5. Smoosh receives the 'DOWN' message, begins the next highest priority job, Shard2 6. Channel concurrency is now 2, not 1 This change adds another check into the 'DOWN' message so that it checks for that specific shard. If the compaction PID exists then it means a new process was spawned and we just monitor that one and add it back to the queue. The length of the queue does not change and therefore we won’t spawn new compaction jobs.
4 tasks
jiangphcn
pushed a commit
that referenced
this pull request
Nov 7, 2020
Smoosh monitors the compactor pid to determine when the compaction jobs finishes, and uses this for its idea of concurrency. However, this isn't accurate in the case where the compaction job has to re-spawn to catch up on intervening changes since the same logical compaction job continues with another pid and smoosh is not aware. In such cases, a smoosh channel with concurrency one can start arbitrarily many additional database compaction jobs. To solve this problem, we added a check to see if a compaction PID exists for a db in `start_compact`. But wee need to add another check because this check is only for shard that comes off the queue. So the following can still occur: 1. Enqueue a bunch of stuff into channel with concurrency 1 2. Begin highest priority job, Shard1, in channel 3. Compaction finishes, discovers compaction file is behind main file 4. Smoosh-monitored PID for Shard1 exits, a new one starts to finish the job 5. Smoosh receives the 'DOWN' message, begins the next highest priority job, Shard2 6. Channel concurrency is now 2, not 1 This change adds another check into the 'DOWN' message so that it checks for that specific shard. If the compaction PID exists then it means a new process was spawned and we just monitor that one and add it back to the queue. The length of the queue does not change and therefore we won’t spawn new compaction jobs.
jiangphcn
added a commit
that referenced
this pull request
Nov 7, 2020
3.x porting - add remonitor code to DOWN message (#3144)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Smoosh monitors the compactor pid to determine when the compaction jobs
finishes, and uses this for its idea of concurrency. However, this isn't
accurate in the case where the compaction job has to re-spawn to catch up on
intervening changes since the same logical compaction job continues with
another pid and smoosh is not aware. In such cases, a smoosh channel with
concurrency one can start arbitrarily many additional database compaction jobs.
To solve this problem, we added a check to see if a compaction PID exists for
a db in
start_compact. But this is the wrong approach because it’s for ashard that comes off the queue. So it’s a different shard and the following
can occur:
Shard2
This change moves the check into the 'DOWN' message so that we can check for
that specific shard. If the compaction PID exists then it means a new process
was spawned and we just monitor that one and add it back to the queue. The
length of the queue does not change and therefore we won’t spawn new
compaction jobs.
Overview
Testing recommendations
Related Issues or Pull Requests
Checklist
rel/overlay/etc/default.ini