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 2020/03/25 20:08:42 UTC
[GitHub] [couchdb] davisp opened a new pull request #2723: Prototype/fdb
layer view cleanup
davisp opened a new pull request #2723: Prototype/fdb layer view cleanup
URL: https://github.com/apache/couchdb/pull/2723
## Overview
Implement `POST /dbname/_view_cleanup` for the fdb-layer
Draft PR cause I haven't written tests yet.
## Testing recommendations
`make check-fdb`
## Checklist
- [ ] Code is written and works correctly
- [ ] Changes are covered by tests
- [x] 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2723:
Prototype/fdb layer view cleanup
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2723: Prototype/fdb layer view cleanup
URL: https://github.com/apache/couchdb/pull/2723#discussion_r406923194
##########
File path: rel/overlay/etc/default.ini
##########
@@ -228,14 +228,17 @@ port = 6984
;fdb_directory = couchdb
;
; Enable or disable index auto-updater
-;index_autoupdater_enabled = true
+;index_updater_enabled = true
Review comment:
Good call
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2723:
Prototype/fdb layer view cleanup
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2723: Prototype/fdb layer view cleanup
URL: https://github.com/apache/couchdb/pull/2723#discussion_r406946289
##########
File path: src/fabric/src/fabric2_index.erl
##########
@@ -54,6 +58,22 @@ db_updated(DbName) when is_binary(DbName) ->
ets:insert_new(Table, {DbName, now_msec()}).
+cleanup(Db) ->
+ try
+ fabric2_fdb:transactional(Db, fun(TxDb) ->
+ DDocs = fabric2_db:get_design_docs(Db),
+ cleanup_indices(TxDb, DDocs)
+ end)
+ catch
+ error:database_does_not_exist ->
+ ok;
+ Tag:Reason ->
+ Stack = erlang:get_stacktrace(),
+ LogMsg = "~p failed to cleanup indices for `~s` ~p:~p ~p",
+ couch_log:error(LogMsg, [?MODULE, Db, Tag, Reason, Stack])
Review comment:
```
io:format("`~s` ~n", [#{x => <<"a">>}]).
** exception error: bad argument
in function io:format/3
called as io:format(<0.64.0>,"`~s` ~n",[#{x => <<"a">>}])
```
Db as a map won't work with `~s`, let's use DbName or `~p` there
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2723:
Prototype/fdb layer view cleanup
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2723: Prototype/fdb layer view cleanup
URL: https://github.com/apache/couchdb/pull/2723#discussion_r406934915
##########
File path: src/couch_views/src/couch_views_jobs.erl
##########
@@ -84,21 +91,32 @@ wait_for_job(JobId, UpdateSeq) ->
end.
-wait_for_job(JobId, Subscription, UpdateSeq) ->
+wait_for_job(JobId, Subscription, DDocId, UpdateSeq) ->
case wait(Subscription) of
+ {not_found, not_found} ->
+ erlang:error(index_not_found);
{error, Error} ->
erlang:error(Error);
+ {finished, #{<<"error">> := <<"ddoc_deleted">>} = Data} ->
+ case maps:get(<<"ddoc_id">>, Data) of
+ DDocId ->
+ couch_jobs:remove(undefined, ?INDEX_JOB_TYPE, JobId),
+ erlang:error({ddoc_deleted, maps:get(<<"reason">>, Data)});
+ _OtherDocId ->
+ retry
Review comment:
This relies on the fact that when it retries it will overwrite the job with its DDocId?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2723:
Prototype/fdb layer view cleanup
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2723: Prototype/fdb layer view cleanup
URL: https://github.com/apache/couchdb/pull/2723#discussion_r406944614
##########
File path: src/couch_views/src/couch_views_jobs.erl
##########
@@ -84,21 +91,35 @@ wait_for_job(JobId, UpdateSeq) ->
end.
-wait_for_job(JobId, Subscription, UpdateSeq) ->
+wait_for_job(JobId, Subscription, DDocId, UpdateSeq) ->
case wait(Subscription) of
+ {not_found, not_found} ->
+ erlang:error(index_not_found);
{error, Error} ->
erlang:error(Error);
+ {finished, #{<<"error">> := <<"ddoc_deleted">>} = Data} ->
+ case maps:get(<<"ddoc_id">>, Data) of
+ DDocId ->
+ couch_jobs:remove(undefined, ?INDEX_JOB_TYPE, JobId),
+ erlang:error({ddoc_deleted, maps:get(<<"reason">>, Data)});
+ _OtherDocId ->
+ % A different design doc wiht the same signature
+ % was deleted. Resubmit this job which will overwrite
+ % the ddoc_id in the job.
+ retry
+ end;
{finished, #{<<"error">> := Error, <<"reason">> := Reason}} ->
+ couch_jobs:remove(undefined, ?INDEX_JOB_TYPE, JobId),
Review comment:
This will work great for `sig_changed` and `db_deleted` but we also fail for any exceptions [1] after we exceed the max retries number or even right away if users picked a large row limit config and they get "tx too long" error. Then current subscribers will get the error but if there is no active request waiting for it and they retry the view later after it finished if we'd want to preserve the failed job?
Maybe we don't because we have some subtle behavior in [2] where if they query the job later, and don't find a `<<"view_seq">>` since it will be an error, they'll get a `retry` will re-add the job and worker will re-build it again.
[1] https://github.com/apache/couchdb/blob/prototype/fdb-layer/src/couch_views/src/couch_views_indexer.erl#L116
[2] https://github.com/apache/couchdb/blob/prototype/fdb-layer/src/couch_views/src/couch_views_jobs.erl#L82
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2723:
Prototype/fdb layer view cleanup
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2723: Prototype/fdb layer view cleanup
URL: https://github.com/apache/couchdb/pull/2723#discussion_r406947871
##########
File path: src/fabric/src/fabric2_index.erl
##########
@@ -54,6 +58,22 @@ db_updated(DbName) when is_binary(DbName) ->
ets:insert_new(Table, {DbName, now_msec()}).
+cleanup(Db) ->
+ try
+ fabric2_fdb:transactional(Db, fun(TxDb) ->
+ DDocs = fabric2_db:get_design_docs(Db),
+ cleanup_indices(TxDb, DDocs)
+ end)
+ catch
+ error:database_does_not_exist ->
+ ok;
+ Tag:Reason ->
+ Stack = erlang:get_stacktrace(),
+ LogMsg = "~p failed to cleanup indices for `~s` ~p:~p ~p",
+ couch_log:error(LogMsg, [?MODULE, Db, Tag, Reason, Stack])
Review comment:
Oh derps. Good catch
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2723:
Prototype/fdb layer view cleanup
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2723: Prototype/fdb layer view cleanup
URL: https://github.com/apache/couchdb/pull/2723#discussion_r406935249
##########
File path: src/couch_views/src/couch_views_jobs.erl
##########
@@ -84,21 +91,32 @@ wait_for_job(JobId, UpdateSeq) ->
end.
-wait_for_job(JobId, Subscription, UpdateSeq) ->
+wait_for_job(JobId, Subscription, DDocId, UpdateSeq) ->
case wait(Subscription) of
+ {not_found, not_found} ->
+ erlang:error(index_not_found);
{error, Error} ->
erlang:error(Error);
+ {finished, #{<<"error">> := <<"ddoc_deleted">>} = Data} ->
+ case maps:get(<<"ddoc_id">>, Data) of
+ DDocId ->
+ couch_jobs:remove(undefined, ?INDEX_JOB_TYPE, JobId),
+ erlang:error({ddoc_deleted, maps:get(<<"reason">>, Data)});
+ _OtherDocId ->
+ retry
Review comment:
Yeah. You reckon I should remove here first?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on a change in pull request #2723:
Prototype/fdb layer view cleanup
Posted by GitBox <gi...@apache.org>.
nickva commented on a change in pull request #2723: Prototype/fdb layer view cleanup
URL: https://github.com/apache/couchdb/pull/2723#discussion_r406938061
##########
File path: src/couch_views/src/couch_views_jobs.erl
##########
@@ -84,21 +91,32 @@ wait_for_job(JobId, UpdateSeq) ->
end.
-wait_for_job(JobId, Subscription, UpdateSeq) ->
+wait_for_job(JobId, Subscription, DDocId, UpdateSeq) ->
case wait(Subscription) of
+ {not_found, not_found} ->
+ erlang:error(index_not_found);
{error, Error} ->
erlang:error(Error);
+ {finished, #{<<"error">> := <<"ddoc_deleted">>} = Data} ->
+ case maps:get(<<"ddoc_id">>, Data) of
+ DDocId ->
+ couch_jobs:remove(undefined, ?INDEX_JOB_TYPE, JobId),
+ erlang:error({ddoc_deleted, maps:get(<<"reason">>, Data)});
+ _OtherDocId ->
+ retry
Review comment:
Maybe a comment that we rely on that fact as it's a bit subtle
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [couchdb] nickva commented on issue #2723: Prototype/fdb layer view
cleanup
Posted by GitBox <gi...@apache.org>.
nickva commented on issue #2723: Prototype/fdb layer view cleanup
URL: https://github.com/apache/couchdb/pull/2723#issuecomment-612226320
```
http http://adm:pass@localhost:15984/deebee/_view_cleanup
HTTP/1.1 202 Accepted
Cache-Control: must-revalidate
Content-Length: 12
Content-Type: application/json
Date: Fri, 10 Apr 2020 21:28:25 GMT
Server: CouchDB/3.0.0-22a7023 (Erlang OTP/21)
X-Couch-Request-ID: 6c451561a6
X-CouchDB-Body-Time: 0
{
"ok": true
}
```
http endpoint works too
Nice remembering to re-activate it after it was switched to return 501s recently
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2723:
Prototype/fdb layer view cleanup
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2723: Prototype/fdb layer view cleanup
URL: https://github.com/apache/couchdb/pull/2723#discussion_r406949810
##########
File path: src/couch_views/src/couch_views_jobs.erl
##########
@@ -84,21 +91,35 @@ wait_for_job(JobId, UpdateSeq) ->
end.
-wait_for_job(JobId, Subscription, UpdateSeq) ->
+wait_for_job(JobId, Subscription, DDocId, UpdateSeq) ->
case wait(Subscription) of
+ {not_found, not_found} ->
+ erlang:error(index_not_found);
{error, Error} ->
erlang:error(Error);
+ {finished, #{<<"error">> := <<"ddoc_deleted">>} = Data} ->
+ case maps:get(<<"ddoc_id">>, Data) of
+ DDocId ->
+ couch_jobs:remove(undefined, ?INDEX_JOB_TYPE, JobId),
+ erlang:error({ddoc_deleted, maps:get(<<"reason">>, Data)});
+ _OtherDocId ->
+ % A different design doc wiht the same signature
+ % was deleted. Resubmit this job which will overwrite
+ % the ddoc_id in the job.
+ retry
+ end;
{finished, #{<<"error">> := Error, <<"reason">> := Reason}} ->
+ couch_jobs:remove(undefined, ?INDEX_JOB_TYPE, JobId),
Review comment:
I agree that the retry is going to retry eventually. This is just to cleanup when a view errors out so we're not leaving orphaned job descriptions everywhere.
As to error conditions in the retry logic anything that comes from there would either be a bug in our code or a transient error that we should eventually be able to get past so I don't think we should be changing things to never remove that job info and always return the same error. (which is not what we're currently doing as you point out).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [couchdb] davisp commented on a change in pull request #2723:
Prototype/fdb layer view cleanup
Posted by GitBox <gi...@apache.org>.
davisp commented on a change in pull request #2723: Prototype/fdb layer view cleanup
URL: https://github.com/apache/couchdb/pull/2723#discussion_r406949910
##########
File path: src/fabric/src/fabric2_index.erl
##########
@@ -54,6 +58,22 @@ db_updated(DbName) when is_binary(DbName) ->
ets:insert_new(Table, {DbName, now_msec()}).
+cleanup(Db) ->
+ try
+ fabric2_fdb:transactional(Db, fun(TxDb) ->
+ DDocs = fabric2_db:get_design_docs(Db),
+ cleanup_indices(TxDb, DDocs)
+ end)
+ catch
+ error:database_does_not_exist ->
+ ok;
+ Tag:Reason ->
+ Stack = erlang:get_stacktrace(),
+ LogMsg = "~p failed to cleanup indices for `~s` ~p:~p ~p",
+ couch_log:error(LogMsg, [?MODULE, Db, Tag, Reason, Stack])
Review comment:
Fixed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [couchdb] davisp merged pull request #2723: Prototype/fdb layer
view cleanup
Posted by GitBox <gi...@apache.org>.
davisp merged pull request #2723: Prototype/fdb layer view cleanup
URL: https://github.com/apache/couchdb/pull/2723
----------------------------------------------------------------
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
With regards,
Apache Git Services