You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2022/07/13 23:43:38 UTC
[GitHub] [couchdb] chewbranca opened a new pull request, #4106: 4101 add io priority
chewbranca opened a new pull request, #4106:
URL: https://github.com/apache/couchdb/pull/4106
<!-- Thank you for your contribution!
Please file this form by replacing the Markdown comments
with your text. If a section needs no action - remove it.
Also remember, that CouchDB uses the Review-Then-Commit (RTC) model
of code collaboration. Positive feedback is represented +1 from committers
and negative is a -1. The -1 also means veto, and needs to be addressed
to proceed. Once there are no objections, the PR can be merged by a
CouchDB committer.
See: http://couchdb.apache.org/bylaws.html#decisions for more info. -->
## Overview
This addresses #4101 and adds `io_priority` labels in a number of places that they weren't previously set before. It also introduces the `system` `io_priority` for internal database operations.
<!-- Please give a short brief for the pull request,
what problem it solves or how it makes things better. -->
I also took the opportunity to add a few utility functions for setting `io_priority` values, abstracting over the use of the process dictionary for storing these values. We could also update the existing places where `io_priority` is set to use the new functions, but that might be better in a separate PR.
## Testing recommendations
<!-- Describe how we can test your changes.
Does it provides any behaviour that the end users
could notice? -->
Normal test suite should suffice.
## Related Issues or Pull Requests
<!-- If your changes affects multiple components in different
repositories please put links to those issues or pull requests here. -->
## Checklist
- [ ] Code is written and works correctly
- [ ] Changes are covered by tests
- [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
- [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] chewbranca commented on pull request #4106: 4101 add io priority
Posted by GitBox <gi...@apache.org>.
chewbranca commented on PR #4106:
URL: https://github.com/apache/couchdb/pull/4106#issuecomment-1194501435
@nickva @jaydoane alright I've added two more commits, one adds IOQ as a dep to the various applications setting IO priority values. The other commit adds a `search` flag for the `io_class` labeling in IOQ utilized in `dreyfus`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4106: 4101 add io priority
Posted by GitBox <gi...@apache.org>.
nickva commented on code in PR #4106:
URL: https://github.com/apache/couchdb/pull/4106#discussion_r920704187
##########
src/couch_index/src/couch_index.erl:
##########
@@ -72,6 +72,7 @@ get_compactor_pid(Pid) ->
init({Mod, IdxState}) ->
DbName = Mod:get(db_name, IdxState),
+ erlang:put(io_priority, {view_update, DbName}),
Review Comment:
should we use `ioq:set_io_priority({system, DbName})` ? here too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4106: 4101 add io priority
Posted by GitBox <gi...@apache.org>.
nickva commented on code in PR #4106:
URL: https://github.com/apache/couchdb/pull/4106#discussion_r920704411
##########
src/chttpd/src/chttpd_auth_cache.erl:
##########
@@ -155,15 +155,17 @@ spawn_changes(Since) ->
Pid.
listen_for_changes(Since) ->
- ensure_auth_ddoc_exists(dbname(), <<"_design/_auth">>),
+ DbName = dbname(),
+ erlang:put(io_priority, {system, DbName}),
Review Comment:
Would `ioq:set_io_priority({system, DbName})` work here (for consistency mainly)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4106: 4101 add io priority
Posted by GitBox <gi...@apache.org>.
nickva commented on code in PR #4106:
URL: https://github.com/apache/couchdb/pull/4106#discussion_r920704187
##########
src/couch_index/src/couch_index.erl:
##########
@@ -72,6 +72,7 @@ get_compactor_pid(Pid) ->
init({Mod, IdxState}) ->
DbName = Mod:get(db_name, IdxState),
+ erlang:put(io_priority, {view_update, DbName}),
Review Comment:
should we use `ioq:set_io_priority({system, DbName})1? here too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4106: 4101 add io priority
Posted by GitBox <gi...@apache.org>.
nickva commented on code in PR #4106:
URL: https://github.com/apache/couchdb/pull/4106#discussion_r920705974
##########
src/chttpd/src/chttpd_auth_cache.erl:
##########
@@ -155,15 +155,17 @@ spawn_changes(Since) ->
Pid.
listen_for_changes(Since) ->
- ensure_auth_ddoc_exists(dbname(), <<"_design/_auth">>),
+ DbName = dbname(),
+ erlang:put(io_priority, {system, DbName}),
Review Comment:
This would be a cluster level db name and in other case we use the shard names. That should be ok, the ioq servers can handle both, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on pull request #4106: 4101 add io priority
Posted by GitBox <gi...@apache.org>.
nickva commented on PR #4106:
URL: https://github.com/apache/couchdb/pull/4106#issuecomment-1184804317
@chewbranca ooh good call about application dependencies. If you feel dependencies would be an issue we can switch back to plain gets and puts as long as it's fairly consistent throughout (say if we'd create some dependency loops or such)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] chewbranca commented on pull request #4106: 4101 add io priority
Posted by GitBox <gi...@apache.org>.
chewbranca commented on PR #4106:
URL: https://github.com/apache/couchdb/pull/4106#issuecomment-1184752330
Thanks @nickva, I forgot to push out the changes cleaning up those remaining pdict puts from the earlier commit.
One thing that occurred to me, setting pdict values is _always_ a valid thing in respect to dependency hierarchies, whereas using `ioq` module functions requires `ioq` to be declared as a dependency. We already do that for the `couch` app https://github.com/apache/couchdb/blob/main/src/couch/src/couch.app.src#L46 but I'll double check that the other places we're doing so also include the `ioq` app in their dependency hierarchy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a diff in pull request #4106: 4101 add io priority
Posted by GitBox <gi...@apache.org>.
nickva commented on code in PR #4106:
URL: https://github.com/apache/couchdb/pull/4106#discussion_r920705049
##########
src/mem3/src/mem3_nodes.erl:
##########
@@ -104,7 +106,7 @@ code_change(_OldVsn, #state{} = State, _Extra) ->
%% internal functions
initialize_nodelist() ->
- DbName = config:get("mem3", "nodes_db", "_nodes"),
+ DbName = mem3_sync:nodes_db(),
Review Comment:
nice!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] chewbranca merged pull request #4106: 4101 add io priority
Posted by GitBox <gi...@apache.org>.
chewbranca merged PR #4106:
URL: https://github.com/apache/couchdb/pull/4106
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org