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 2021/03/01 20:36:32 UTC
[GitHub] [couchdb] nickva opened a new pull request #3391: [wip] use indexer GRV to read view builds
nickva opened a new pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391
If the view build was triggered because it was stale and finishes during the
request, used the indexer GRV (i.e. the state of the indexer when it read the
database and thus the doc bodies) to emit the view. This way the docs emitted
with include_docs=true will match the revisions used when emitted the view
values.
Issue: https://github.com/apache/couchdb/issues/3381
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#discussion_r590581733
##########
File path: src/couch_views/src/couch_views_indexer.erl
##########
@@ -258,7 +260,24 @@ do_update(Db, Mrst0, State0) ->
update_stats := UpdateStats
}}
end
- end).
+ end),
+ case TxResult of
+ {Mrst, continue, State} ->
+ {Mrst, State};
+ {Mrst, finished, State} ->
+ #{tx_db := OldDb} = State,
+ CommittedVer = erlfdb:get_committed_version(maps:get(tx, OldDb)),
+ fabric2_fdb:transactional(OldDb#{tx := undefined}, fun(TxDb) ->
Review comment:
Fixed in d5fae35f695758a2b21bf8936f28e5c310f894c4
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] kocolosk commented on a change in pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#discussion_r588890781
##########
File path: src/couch_views/src/couch_views.erl
##########
@@ -126,14 +127,29 @@ get_total_view_size(TxDb, Mrst) ->
end, 0, Mrst#mrst.views).
-read_view(Db, Mrst, ViewName, Callback, Acc0, Args) ->
+read_view(Db, Mrst, ViewName, Callback, Acc0, Args, {_, _} = IdxVStamps) ->
+ {IndexerGRV, CommittedVer} = IdxVStamps,
Review comment:
Here and elsewhere in the `read_view` code path I think it might help future devs to use names that more directly indicate what the purpose of these two versions are, e.g. `DbReadVsn` and `ViewReadVsn` or similar.
##########
File path: src/couch_views/src/couch_views_indexer.erl
##########
@@ -258,7 +260,24 @@ do_update(Db, Mrst0, State0) ->
update_stats := UpdateStats
}}
end
- end).
+ end),
+ case TxResult of
+ {Mrst, continue, State} ->
+ {Mrst, State};
+ {Mrst, finished, State} ->
+ #{tx_db := OldDb} = State,
+ CommittedVer = erlfdb:get_committed_version(maps:get(tx, OldDb)),
+ fabric2_fdb:transactional(OldDb#{tx := undefined}, fun(TxDb) ->
Review comment:
It'd be good to avoid the roundtrip to acquire a new read version here. Can we configure the new transaction to use the previous one's committed version as its read version?
##########
File path: src/couch_views/src/couch_views.erl
##########
@@ -53,11 +53,12 @@ query(Db, DDoc, ViewName, Callback, Acc0, Args0) ->
try
fabric2_fdb:transactional(Db, fun(TxDb) ->
ok = maybe_update_view(TxDb, Mrst, IsInteractive, Args3),
- read_view(TxDb, Mrst, ViewName, Callback, Acc0, Args3)
+ IdxVStamps = {current, current},
+ read_view(TxDb, Mrst, ViewName, Callback, Acc0, Args3, IdxVStamps)
end)
catch throw:{build_view, WaitSeq} ->
- couch_views_jobs:build_view(Db, Mrst, WaitSeq),
- read_view(Db, Mrst, ViewName, Callback, Acc0, Args3)
+ {ok, IdxVStamps} = couch_views_jobs:build_view(Db, Mrst, WaitSeq),
+ read_view(Db, Mrst, ViewName, Callback, Acc0, Args3, IdxVStamps)
Review comment:
I don't know the answer to this question, but is there an extra unneeded GRV roundtrip here? The `read_view` function is going to `fabric2_fdb:transactional` to start a transaction, but we already have the read version we want to use so a roundtrip to FDB to acquire one would be a waste. I just don't know if that GRV operation happens before we set the read version inside the transaction.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
nickva commented on pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#issuecomment-797265211
Found one more test that was failing because of how it mocked the indexer and expected do_update to be called. Fixed in a separate commit 771bd2c4d69c950db8bc2f807bbda3f84f3b4a0c
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva merged pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
nickva merged pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#discussion_r590560398
##########
File path: src/couch_views/src/couch_views_indexer.erl
##########
@@ -258,7 +260,24 @@ do_update(Db, Mrst0, State0) ->
update_stats := UpdateStats
}}
end
- end).
+ end),
+ case TxResult of
+ {Mrst, continue, State} ->
+ {Mrst, State};
+ {Mrst, finished, State} ->
+ #{tx_db := OldDb} = State,
+ CommittedVer = erlfdb:get_committed_version(maps:get(tx, OldDb)),
+ fabric2_fdb:transactional(OldDb#{tx := undefined}, fun(TxDb) ->
Review comment:
Since we're re-using the GRV from the last `do_update` call, another transaction might have read something that we forgot to snapshot in our `do_update` transaction and then did any writes before this second finalize transaction ran. Though we do read the changes and doc bodies with snapshots now, so that leaves the `couch_views_fdb:get_view_state/2` call but maybe others (or new ones added in the future).. That would be a spurious (un-intended conflict) in way and we'd be stuck in a loop.
> could we set it to ?INDEX_READY in a retry after another transaction (e.g. the next view build) has configured it differently?
Good question. Build state should be set by the indexer only. And only one indexer may operate on the job at the time. That's exactly what the `couch_jobs_fdb:update/3` accomplishes. The logic in update/3 as follows:
* `get` the job data and state
* If the lock uuid doesn't match the one read, then exit with `halt`. This means someone else might have acquired the job or it was deleted, etc.
* Otherwise, `put` the job data and then bump the activity (both the watch for the whole job type and the individual jobs)
So nobody else should be clobbering the index_ready state, as long as the current job is holding the job lock. And it may hold the lock across multiple transactions. For that to work though, indexers _must_ call `couch_jobs:update/3` every time they update the index (that happens in `report_progress/2`). Which means in the current PR there is a bug because during the last do_update call we remove the call `report_progress/2`(I thought it would be an optimization, ha!). Good eye, @kocolosk!
It seems then we need to ensure we call progress update in the current PR on the last do_update calls, but maybe also keep the opportunistic GRV set to prevent possible spurious conflicts.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva edited a comment on pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
nickva edited a comment on pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#issuecomment-797265211
Found one more test that was failing because of how it mocked the indexer and expected do_update to be called. Fixed in a separate commit ea36569
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#discussion_r588900917
##########
File path: src/couch_views/src/couch_views.erl
##########
@@ -53,11 +53,12 @@ query(Db, DDoc, ViewName, Callback, Acc0, Args0) ->
try
fabric2_fdb:transactional(Db, fun(TxDb) ->
ok = maybe_update_view(TxDb, Mrst, IsInteractive, Args3),
- read_view(TxDb, Mrst, ViewName, Callback, Acc0, Args3)
+ IdxVStamps = {current, current},
+ read_view(TxDb, Mrst, ViewName, Callback, Acc0, Args3, IdxVStamps)
end)
catch throw:{build_view, WaitSeq} ->
- couch_views_jobs:build_view(Db, Mrst, WaitSeq),
- read_view(Db, Mrst, ViewName, Callback, Acc0, Args3)
+ {ok, IdxVStamps} = couch_views_jobs:build_view(Db, Mrst, WaitSeq),
+ read_view(Db, Mrst, ViewName, Callback, Acc0, Args3, IdxVStamps)
Review comment:
Good point. I was worried about that too. Inside the transaction closure we set the GRV before we do any reads or writes. In case the GRV is already acquired we should get a 2010 (read_version_already_set) error that should serve as an assertion. I observed those errors a few times when running experiments with this PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#discussion_r588975961
##########
File path: src/couch_views/src/couch_views.erl
##########
@@ -126,14 +127,29 @@ get_total_view_size(TxDb, Mrst) ->
end, 0, Mrst#mrst.views).
-read_view(Db, Mrst, ViewName, Callback, Acc0, Args) ->
+read_view(Db, Mrst, ViewName, Callback, Acc0, Args, {_, _} = IdxVStamps) ->
+ {IndexerGRV, CommittedVer} = IdxVStamps,
Review comment:
Updated in a [fixup]( https://github.com/apache/couchdb/pull/3391/commits/740eb08df620386b72a941ad422152715b633e45)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] kocolosk commented on a change in pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#discussion_r589921711
##########
File path: src/couch_views/src/couch_views_indexer.erl
##########
@@ -258,7 +260,24 @@ do_update(Db, Mrst0, State0) ->
update_stats := UpdateStats
}}
end
- end).
+ end),
+ case TxResult of
+ {Mrst, continue, State} ->
+ {Mrst, State};
+ {Mrst, finished, State} ->
+ #{tx_db := OldDb} = State,
+ CommittedVer = erlfdb:get_committed_version(maps:get(tx, OldDb)),
+ fabric2_fdb:transactional(OldDb#{tx := undefined}, fun(TxDb) ->
Review comment:
So I agree we want to avoid getting into an infinite retry loop if the committed version is unusable.
Are edit conflicts a real possibility here? The `set_build_status` function is doing a blind write on the key associated with this view group, so it will never conflict on its own. But if there is some contention on the view group then I worry that mutating this key blindly could have issues, e.g. could we set it to `?INDEX_READY` in a retry after another transaction (e.g. the next view build) has configured it differently?
The code in `couch_jobs_fdb:update/3` is a bit too dense for me to follow easily. Would there possibly be other contended updates on the key range associated with this job? Or is this transaction just reading other keys that could be updated elsewhere and bumping into conflicts that 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
nickva commented on pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#issuecomment-794586381
Two more fixups:
* https://github.com/apache/couchdb/pull/3391/commits/a0cc49fc8af1c567c7362c44163827f99b7f4b13 : ensure that we don't bump the `DbReadVsn` but not the `DbViewVns` as it makes the logic of view emits a bit more complicated if we do want to emit from an un-completed view. So we ensure we persist either both `DbReadVsn` and `ViewReadVsn` together or they both stay `null`.
* https://github.com/apache/couchdb/pull/3391/commits/c0b24dacf0f26ac23bc8e874ea83573567525f3b This is a cherry pick from @kocolosk. This is needed to pass all the tests otherwise the last do_update's report_progress call will result in a data update with `ViewSeq >= UpdateSeq` which will make subscriptions fire break a bunch of unit tests.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#discussion_r588976781
##########
File path: src/couch_views/src/couch_views_indexer.erl
##########
@@ -258,7 +260,24 @@ do_update(Db, Mrst0, State0) ->
update_stats := UpdateStats
}}
end
- end).
+ end),
+ case TxResult of
+ {Mrst, continue, State} ->
+ {Mrst, State};
+ {Mrst, finished, State} ->
+ #{tx_db := OldDb} = State,
+ CommittedVer = erlfdb:get_committed_version(maps:get(tx, OldDb)),
+ fabric2_fdb:transactional(OldDb#{tx := undefined}, fun(TxDb) ->
Review comment:
Implemented in a [fixup]( https://github.com/apache/couchdb/pull/3391/commits/c29f5b24437e0cddc1632586084b4698ff175edd)
`erlfdb:get_last_error/0` made it quite easy to check if the transactional closure is being retried due to an error.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] kocolosk commented on pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
kocolosk commented on pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#issuecomment-797164160
Agree that would be a separate PR. I'm fine to leave 2e40063 out as well -- it's not related to the work of getting `include_docs` to match the view output, it's on a different topic of ensuring the view actually observes a snapshot of the DB as it existed at some point in time (which is only the case today if the indexer does not need to retry, or the write pattern is insert-only).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#discussion_r590560398
##########
File path: src/couch_views/src/couch_views_indexer.erl
##########
@@ -258,7 +260,24 @@ do_update(Db, Mrst0, State0) ->
update_stats := UpdateStats
}}
end
- end).
+ end),
+ case TxResult of
+ {Mrst, continue, State} ->
+ {Mrst, State};
+ {Mrst, finished, State} ->
+ #{tx_db := OldDb} = State,
+ CommittedVer = erlfdb:get_committed_version(maps:get(tx, OldDb)),
+ fabric2_fdb:transactional(OldDb#{tx := undefined}, fun(TxDb) ->
Review comment:
Since we're re-using the GRV from the last `do_update` call, another transaction might have read something that we forgot to snapshot in our `do_update` transaction and then did any writes before this second finalize transaction ran. Though we do read the changes and doc bodies with snapshots now, so that leaves the `couch_views_fdb:get_view_state/2` call but maybe others (or news ones added in the future).. That would be a spurious (un-intended conflict) in way and we'd be stuck in a loop.
> could we set it to ?INDEX_READY in a retry after another transaction (e.g. the next view build) has configured it differently?
Good question. Build state should be set by the indexer only. And only one indexer may operate on the job at the time. That's exactly what the `couch_jobs_fdb:update/3` accomplishes. The logic in update/3 as follows:
* `get` the job data and state
* If the lock uuid doesn't match the one read, then exit with `halt`. This means someone else might have acquired the job or it was deleted, etc.
* Otherwise, `put` the job data and then bump the activity (both the watch for the whole job type and the individual jobs)
So nobody else should be clobbering the index_ready state, as long as the current job is holding the job lock. And it may hold the lock across multiple transactions. For that to work though, indexers _must_ call `couch_jobs:update/3` every time they update the index (that happens in `report_progress/2`). Which means in the current PR there is a bug because during the last do_update call we remove the call `report_progress/2`(I thought it would be an optimization, ha!). Good eye, @kocolosk!
It seems then we need to ensure we call progress update in the current PR on the last do_update calls, but maybe also keep the opportunistic GRV set to prevent possible spurious conflicts.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#discussion_r588975961
##########
File path: src/couch_views/src/couch_views.erl
##########
@@ -126,14 +127,29 @@ get_total_view_size(TxDb, Mrst) ->
end, 0, Mrst#mrst.views).
-read_view(Db, Mrst, ViewName, Callback, Acc0, Args) ->
+read_view(Db, Mrst, ViewName, Callback, Acc0, Args, {_, _} = IdxVStamps) ->
+ {IndexerGRV, CommittedVer} = IdxVStamps,
Review comment:
Updated in a fixup
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
nickva commented on pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#issuecomment-797010959
Thanks for taking a look @kocolosk. I integrated c0b24da to make the test suite pass. Should I integrate 2e40063 as well? I'd be happy to do it
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#discussion_r588899156
##########
File path: src/couch_views/src/couch_views.erl
##########
@@ -126,14 +127,29 @@ get_total_view_size(TxDb, Mrst) ->
end, 0, Mrst#mrst.views).
-read_view(Db, Mrst, ViewName, Callback, Acc0, Args) ->
+read_view(Db, Mrst, ViewName, Callback, Acc0, Args, {_, _} = IdxVStamps) ->
+ {IndexerGRV, CommittedVer} = IdxVStamps,
Review comment:
Agree, those are better names. I will update those.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] kocolosk commented on a change in pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
kocolosk commented on a change in pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#discussion_r592643870
##########
File path: src/couch_views/src/couch_views_indexer.erl
##########
@@ -258,7 +260,24 @@ do_update(Db, Mrst0, State0) ->
update_stats := UpdateStats
}}
end
- end).
+ end),
+ case TxResult of
+ {Mrst, continue, State} ->
+ {Mrst, State};
+ {Mrst, finished, State} ->
+ #{tx_db := OldDb} = State,
+ CommittedVer = erlfdb:get_committed_version(maps:get(tx, OldDb)),
+ fabric2_fdb:transactional(OldDb#{tx := undefined}, fun(TxDb) ->
Review comment:
You're giving me entirely too much credit on that one, I had absolutely no idea about the `couch_jobs:update/3` contract :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#discussion_r592648037
##########
File path: src/couch_views/src/couch_views_indexer.erl
##########
@@ -258,7 +260,24 @@ do_update(Db, Mrst0, State0) ->
update_stats := UpdateStats
}}
end
- end).
+ end),
+ case TxResult of
+ {Mrst, continue, State} ->
+ {Mrst, State};
+ {Mrst, finished, State} ->
+ #{tx_db := OldDb} = State,
+ CommittedVer = erlfdb:get_committed_version(maps:get(tx, OldDb)),
+ fabric2_fdb:transactional(OldDb#{tx := undefined}, fun(TxDb) ->
Review comment:
You had the right intuition to ask about contended updates for the key range of that job!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
nickva commented on pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#issuecomment-797012667
Another piece that would be related is to modify the couch_views batcher so it doesn't try to bump against the 5 second timeout limit and instead use a lower limit (you suggested 1 sec). I think it would be a separate PR though
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] nickva commented on a change in pull request #3391: Consistent view emits using indexer's GRVs and committed versionstamps
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #3391:
URL: https://github.com/apache/couchdb/pull/3391#discussion_r588899122
##########
File path: src/couch_views/src/couch_views_indexer.erl
##########
@@ -258,7 +260,24 @@ do_update(Db, Mrst0, State0) ->
update_stats := UpdateStats
}}
end
- end).
+ end),
+ case TxResult of
+ {Mrst, continue, State} ->
+ {Mrst, State};
+ {Mrst, finished, State} ->
+ #{tx_db := OldDb} = State,
+ CommittedVer = erlfdb:get_committed_version(maps:get(tx, OldDb)),
+ fabric2_fdb:transactional(OldDb#{tx := undefined}, fun(TxDb) ->
Review comment:
That could work. Good idea. We already have a recent GRV so we could use it. However, since this will be a write transaction there is a risk to be stuck in a conflict loop here (job data values could be fairly contended). But we could set it up as an opportunistic GRV set, such that we try once only then ignore it on an error retry (with a pdict trick).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org