You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by "nickva (via GitHub)" <gi...@apache.org> on 2025/10/27 20:48:00 UTC
[PR] Use W=N for auto-purge plugin [couchdb]
nickva opened a new pull request, #5716:
URL: https://github.com/apache/couchdb/pull/5716
The idea is we would like to do as much work in the auto-purge request itself and leave as little as possible for the internal replicator.
While at it, strengthen some state map matches by asserting that we expect the fields to always by there. Also, ensure to always reset queue to empty as when we're opening a new db shard.
--
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
Re: [PR] Use W=N for auto-purge plugin [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5716:
URL: https://github.com/apache/couchdb/pull/5716#discussion_r2467073881
##########
src/couch/src/couch_auto_purge_plugin.erl:
##########
@@ -47,7 +47,7 @@ checkpoint(_St) ->
db(St, DbName) ->
case ttl(St, DbName) of
TTL when is_integer(TTL) ->
- {ok, St#{ttl => TTL}};
+ {ok, St#{ttl => TTL, db_name => DbName, db_n => mem3:n(DbName)}};
Review Comment:
The idea was to save some resource on `n` lookup since it's a `mem3:shards/1` call. That's normally an ets lookup, but it also sends a cache hit message to the cache gen_server. But it's probably not a big deal?
--
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
Re: [PR] Use W=N for auto-purge plugin [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #5716:
URL: https://github.com/apache/couchdb/pull/5716#discussion_r2468789140
##########
src/couch/src/couch_auto_purge_plugin.erl:
##########
@@ -64,9 +65,9 @@ db_opened(#{} = St, Db) ->
{0, ChangeOpts, St#{count => 0, end_seq => EndSeq}}.
db_closing(#{} = St, Db) ->
- #{count := Count} = flush_queue(St, Db),
- ?INFO("purged ~B deleted documents from ~s", [Count, couch_db:name(Db)], meta(St)),
- {ok, St}.
+ St1 = #{count := Count} = flush_queue(St, 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
Re: [PR] Use W=N for auto-purge plugin [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #5716:
URL: https://github.com/apache/couchdb/pull/5716#discussion_r2467095956
##########
src/couch/src/couch_auto_purge_plugin.erl:
##########
@@ -61,7 +61,7 @@ db_opened(#{} = St, Db) ->
true -> [{end_key, EndSeq}]
end,
?INFO("scanning for deleted documents in ~s up to ~p", [couch_db:name(Db), EndSeq], meta(St)),
- {0, ChangeOpts, St#{count => 0, end_seq => EndSeq}}.
+ {0, ChangeOpts, St#{count => 0, end_seq => EndSeq, queue := []}}.
Review Comment:
yes. have flush_queue always set queue to [] but note in the error cases why that's ok (because we'll try again on the next run).
--
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
Re: [PR] Use W=N for auto-purge plugin [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #5716:
URL: https://github.com/apache/couchdb/pull/5716#discussion_r2467049058
##########
src/couch/src/couch_auto_purge_plugin.erl:
##########
@@ -61,7 +61,7 @@ db_opened(#{} = St, Db) ->
true -> [{end_key, EndSeq}]
end,
?INFO("scanning for deleted documents in ~s up to ~p", [couch_db:name(Db), EndSeq], meta(St)),
- {0, ChangeOpts, St#{count => 0, end_seq => EndSeq}}.
+ {0, ChangeOpts, St#{count => 0, end_seq => EndSeq, queue := []}}.
Review Comment:
I'd prefer an assertion that it was already empty, since we always flush the queue in db_closing. e.g a match in the function head on `queue := []`.
##########
src/couch/src/couch_auto_purge_plugin.erl:
##########
@@ -89,19 +89,18 @@ enqueue(#{queue := Queue} = St0, Id, Revs, Db) ->
if
NewQueueSize > MaxBatchSize ->
{RevBatch, RevRest} = lists:split(MaxBatchSize - CurrentQueueSize, Revs),
- St1 = flush_queue(St0#{queue => [{Id, RevBatch} | Queue]}, Db),
+ St1 = flush_queue(St0#{queue := [{Id, RevBatch} | Queue]}, Db),
enqueue(St1, Id, RevRest, Db);
NewQueueSize >= MinBatchSize ->
- flush_queue(St0#{queue => [{Id, Revs} | Queue]}, Db);
+ flush_queue(St0#{queue := [{Id, Revs} | Queue]}, Db);
Review Comment:
good idea on asserting these should already exist
##########
src/couch/src/couch_auto_purge_plugin.erl:
##########
@@ -89,19 +89,18 @@ enqueue(#{queue := Queue} = St0, Id, Revs, Db) ->
if
NewQueueSize > MaxBatchSize ->
{RevBatch, RevRest} = lists:split(MaxBatchSize - CurrentQueueSize, Revs),
- St1 = flush_queue(St0#{queue => [{Id, RevBatch} | Queue]}, Db),
+ St1 = flush_queue(St0#{queue := [{Id, RevBatch} | Queue]}, Db),
enqueue(St1, Id, RevRest, Db);
NewQueueSize >= MinBatchSize ->
- flush_queue(St0#{queue => [{Id, Revs} | Queue]}, Db);
+ flush_queue(St0#{queue := [{Id, Revs} | Queue]}, Db);
NewQueueSize < MinBatchSize ->
- St0#{queue => [{Id, Revs} | Queue]}
+ St0#{queue := [{Id, Revs} | Queue]}
end.
flush_queue(#{queue := []} = St, _Db) ->
St;
-flush_queue(#{queue := IdRevs} = St, Db) ->
- DbName = mem3:dbname(couch_db:name(Db)),
- PurgeFun = fun() -> fabric:purge_docs(DbName, IdRevs, [?ADMIN_CTX]) end,
+flush_queue(#{queue := IdRevs, db_name := DbName, db_n := N} = St, Db) ->
+ PurgeFun = fun() -> fabric:purge_docs(DbName, IdRevs, [?ADMIN_CTX, {w, N}]) end,
Review Comment:
agree on goal but would like to see this tidier
##########
src/couch/src/couch_auto_purge_plugin.erl:
##########
@@ -47,7 +47,7 @@ checkpoint(_St) ->
db(St, DbName) ->
case ttl(St, DbName) of
TTL when is_integer(TTL) ->
- {ok, St#{ttl => TTL}};
+ {ok, St#{ttl => TTL, db_name => DbName, db_n => mem3:n(DbName)}};
Review Comment:
I'd rather not clutter the state with information that the callbacks already include, it's just more things to go wrong and we have to maintain.
--
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
Re: [PR] Use W=N for auto-purge plugin [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #5716:
URL: https://github.com/apache/couchdb/pull/5716#discussion_r2467089720
##########
src/couch/src/couch_auto_purge_plugin.erl:
##########
@@ -47,7 +47,7 @@ checkpoint(_St) ->
db(St, DbName) ->
case ttl(St, DbName) of
TTL when is_integer(TTL) ->
- {ok, St#{ttl => TTL}};
+ {ok, St#{ttl => TTL, db_name => DbName, db_n => mem3:n(DbName)}};
Review Comment:
I get that, but mem3:shards reads cached data and we only flush when we reach or exceed min_batch_size. perhaps mem3:shards should move from ETS to persistent_term given how infrequently the shard map of a given database changes?
--
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
Re: [PR] Use W=N for auto-purge plugin [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5716:
URL: https://github.com/apache/couchdb/pull/5716#discussion_r2467073881
##########
src/couch/src/couch_auto_purge_plugin.erl:
##########
@@ -47,7 +47,7 @@ checkpoint(_St) ->
db(St, DbName) ->
case ttl(St, DbName) of
TTL when is_integer(TTL) ->
- {ok, St#{ttl => TTL}};
+ {ok, St#{ttl => TTL, db_name => DbName, db_n => mem3:n(DbName)}};
Review Comment:
The idea was to save some resource on `n` lookup since it's a `mem3:shards`, that's normally an ets lookup it also sends a cache hit message to the cache gen_server. But it's probably not a big deal wither way
--
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
Re: [PR] Use W=N for auto-purge plugin [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5716:
URL: https://github.com/apache/couchdb/pull/5716#discussion_r2467090013
##########
src/couch/src/couch_auto_purge_plugin.erl:
##########
@@ -61,7 +61,7 @@ db_opened(#{} = St, Db) ->
true -> [{end_key, EndSeq}]
end,
?INFO("scanning for deleted documents in ~s up to ~p", [couch_db:name(Db), EndSeq], meta(St)),
- {0, ChangeOpts, St#{count => 0, end_seq => EndSeq}}.
+ {0, ChangeOpts, St#{count => 0, end_seq => EndSeq, queue := []}}.
Review Comment:
Yeah, I think it would be better to explicitly reset it in flush and then assert queue is empty on db closing?
--
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
Re: [PR] Use W=N for auto-purge plugin [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva merged PR #5716:
URL: https://github.com/apache/couchdb/pull/5716
--
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
Re: [PR] Use W=N for auto-purge plugin [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5716:
URL: https://github.com/apache/couchdb/pull/5716#discussion_r2467068588
##########
src/couch/src/couch_auto_purge_plugin.erl:
##########
@@ -61,7 +61,7 @@ db_opened(#{} = St, Db) ->
true -> [{end_key, EndSeq}]
end,
?INFO("scanning for deleted documents in ~s up to ~p", [couch_db:name(Db), EndSeq], meta(St)),
- {0, ChangeOpts, St#{count => 0, end_seq => EndSeq}}.
+ {0, ChangeOpts, St#{count => 0, end_seq => EndSeq, queue := []}}.
Review Comment:
Do we always flush it though? It looked like on error we may leave some in the queue
--
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
Re: [PR] Use W=N for auto-purge plugin [couchdb]
Posted by "rnewson (via GitHub)" <gi...@apache.org>.
rnewson commented on code in PR #5716:
URL: https://github.com/apache/couchdb/pull/5716#discussion_r2467087930
##########
src/couch/src/couch_auto_purge_plugin.erl:
##########
@@ -61,7 +61,7 @@ db_opened(#{} = St, Db) ->
true -> [{end_key, EndSeq}]
end,
?INFO("scanning for deleted documents in ~s up to ~p", [couch_db:name(Db), EndSeq], meta(St)),
- {0, ChangeOpts, St#{count => 0, end_seq => EndSeq}}.
+ {0, ChangeOpts, St#{count => 0, end_seq => EndSeq, queue := []}}.
Review Comment:
my intention was that we always flush everything when we switch databases, rather than have the batching go across them (and specifically to keep things simpler and avoiding making duplicate data like the dbname in the state when it's in all the callbacks). so if that _isn't_ happening we should fix it so it does.
--
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
Re: [PR] Use W=N for auto-purge plugin [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5716:
URL: https://github.com/apache/couchdb/pull/5716#discussion_r2467090560
##########
src/couch/src/couch_auto_purge_plugin.erl:
##########
@@ -47,7 +47,7 @@ checkpoint(_St) ->
db(St, DbName) ->
case ttl(St, DbName) of
TTL when is_integer(TTL) ->
- {ok, St#{ttl => TTL}};
+ {ok, St#{ttl => TTL, db_name => DbName, db_n => mem3:n(DbName)}};
Review Comment:
I'll update it to avoid the extra state
--
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
Re: [PR] Use W=N for auto-purge plugin [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5716:
URL: https://github.com/apache/couchdb/pull/5716#discussion_r2467073881
##########
src/couch/src/couch_auto_purge_plugin.erl:
##########
@@ -47,7 +47,7 @@ checkpoint(_St) ->
db(St, DbName) ->
case ttl(St, DbName) of
TTL when is_integer(TTL) ->
- {ok, St#{ttl => TTL}};
+ {ok, St#{ttl => TTL, db_name => DbName, db_n => mem3:n(DbName)}};
Review Comment:
The idea was to save some resource on `n` lookup since it's a `mem3:shards`, that's normally an ets lookup it also sends a cache hit message to the cache gen_server. But it's probably not a big deal?
--
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
Re: [PR] Use W=N for auto-purge plugin [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5716:
URL: https://github.com/apache/couchdb/pull/5716#discussion_r2467749698
##########
src/couch/src/couch_auto_purge_plugin.erl:
##########
@@ -64,9 +65,9 @@ db_opened(#{} = St, Db) ->
{0, ChangeOpts, St#{count => 0, end_seq => EndSeq}}.
db_closing(#{} = St, Db) ->
- #{count := Count} = flush_queue(St, Db),
- ?INFO("purged ~B deleted documents from ~s", [Count, couch_db:name(Db)], meta(St)),
- {ok, St}.
+ St1 = #{count := Count} = flush_queue(St, Db),
Review Comment:
This was another bug (missing the updated state)found by the assert. Good idea to add it! Pushed a fix
--
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