-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set io_priority in all IO paths #4101
Comments
@chewbranca good idea to have better priority classes! Replicator db access from the replicator could be a
I think that's just a left-over clause we didn't clean up when we removed the |
Yeah I think I do like the idea of being able to prioritize replicator traffic at a different priority level than
Makes sense 👍 |
I've got two PRs out for this, one in the main CouchDB repo #4106 and a corresponding PR in the IOQ repo: apache/couchdb-ioq#19 |
Can this be closed? |
Summary
There are a number of codepaths performing IO operations that do not set an
io_priority
flag, so they do not properly get labeled or prioritized by IOQ and default toother
io_class [1]. Many moons ago I made a PR [2] that went through and properly tagged anywhere I could find making IO requests without anio_priority
set. It looks like the main gaps around indexing and indexer compaction were fixed in [3], however there are still a number of places lacking anio_priority
value.I also think it would be worthwhile to introduce at least one, perhaps two, additional IO classes. I think we should add a
system
io_priority class for things like auth cache, loading mem3 shards, global changes, couch_per_user, etc. I think it might also be worthwhile to add a dedicatedreplication
io_priority, although it kind of looks like the replicator no longer does local db replications? In [2], the replicator still allowed for doing a directcouch_changes
fetch of the local database, so perhaps my original concern around the replicator doing direct changes reads of full databases going unflagged with io_priority is no longer relevant. We should probably still tag the replicator doc updates with eithersystem
orreplication
though.Desired Behaviour
To minimize (or eliminate) the use of the fallback
other
io_priority class so that all IO is properly flagged with the appropriate IO class.Possible Solution
The PR in [2] has a number of simple additions of where to put the
io_priority
process dictionary value when missing. I tracked those down by modifying the code to throw an exception when absent and then iterated through all the locations I could find. I found many, but the approach was not fully exhaustive so I don't think we should crash by default when there's noio_priority
set.Additional context
[1] https://github.com/apache/couchdb/blob/main/src/ioq/src/ioq.erl#L84-L85
[2] https://github.com/apache/couchdb/pull/1998/files
[3] 13bf0eb
The text was updated successfully, but these errors were encountered: