Conversation
Add test for view after purge requests BugzID: 68276
couch_db:purge_docs now accepts a list of IdRevs this changes in the tests calls this new API BugzID: 68276
BugzID: 68276
|
LGTM though we can't merge this until after PSE stuff lands of course. I just have a few minor style tweaks to note but otherwise this looks pretty good all around. |
davisp
left a comment
davisp
left a comment
There was a problem hiding this comment.
Very good PR over all, just a few minor tweaks all related to style.
| % delete local purge document if have | ||
| [PureFN] = lists:nthtail(length(FilePathList)-1, FilePathList), | ||
| PureFNExt = filename:extension(PureFN), | ||
| Sig = filename:basename(PureFN, PureFNExt), |
There was a problem hiding this comment.
You should take the last four or five lines and move them to a couch_mrview_util function called get_signature_from_filename or something similar. We should also probably assert that the signature is something close to expected, ie > 16 characters with only hexadecimal characters or something.
| false | ||
| end. | ||
|
|
||
|
|
There was a problem hiding this comment.
You've got two extra blank lines here.
| -include_lib("couch/include/couch_db.hrl"). | ||
| -include_lib("couch_mrview/include/couch_mrview.hrl"). | ||
|
|
||
| utc_string() -> |
There was a problem hiding this comment.
Missing a blank line before utc_string/0
| {<<"indexname">>, get(idx_name, State)}, | ||
| {<<"signature">>, Sig} | ||
| ]}}, | ||
| {<<"type">>, <<"view">>} |
There was a problem hiding this comment.
I would use <<"mrview">> instead of just <<"view">>.
|
|
||
|
|
||
| get_local_purge_doc_id(Sig) -> | ||
| list_to_binary(?LOCAL_DOC_PREFIX ++ "purge-view-" ++ Sig). |
There was a problem hiding this comment.
I would use purge-mrview instead of purge-view
|
Thanks, Paul. I already addressed all comments above in commit 4a1d645. Please help check when you get time. Regarding |
| case couch_db:open_int(ShardDbName, []) of | ||
| {ok, Db} -> | ||
| try | ||
| DbName =mem3:dbname(Db#db.name), |
|
|
||
| maybe_create_local_purge_doc(Db, State) -> | ||
| Sig = couch_index_util:hexsig(get(signature, State)), | ||
| case couch_db:open_doc(Db, couch_mrview_util:get_local_purge_doc_id(Sig), []) of |
There was a problem hiding this comment.
Line is longer than 80 characters.
|
|
||
| get_signature_from_filename(FileName) -> | ||
| FilePathList = filename:split(FileName), | ||
| [PureFN] = lists:nthtail(length(FilePathList)-1, FilePathList), |
There was a problem hiding this comment.
Missing spaces around - operator.
| ]}, | ||
| ?assertEqual(Expect, Result), | ||
|
|
||
| % 1st purge request |
| {<<"verify_function">>, <<"verify_index_exists">>}, | ||
| {<<"verify_options">>, {[ | ||
| {<<"dbname">>, get(db_name, State)}, | ||
| {<<"indexname">>, get(idx_name, State)}, |
There was a problem hiding this comment.
you should call this ddoc_id here since that'll be more consistent with other parts of the code base. For other indexers that don't treat the whole ddoc as a single index is where there's an added indexname.
| catch E:T -> | ||
| Stack = erlang:get_stacktrace(), | ||
| couch_log:error("Error occurs when verifying existence of index: ~p ~p", | ||
| [{E, T}, Stack]), |
There was a problem hiding this comment.
I would change that to ... "verifying existince of ~s/~s :: ~p ~p", [ShardDbName, DDocId, {E, T}, Stack]
- implementation of updating local purge document - provide verify_index_exists/1 to check whether index exists or not - clean up local purge document in cleanup cycle - add new test case and refine existing test cases - create local purge document if it doesn’t exist when couch_mrview_index:open/2 is called - directly use db to clean up local purge document - use couch_db:purge_docs/2 with UUID - add error handling for verify_index_exists/1 - add check for Sig in couch_mrview_cleanup:run/1 - construct get_signature_from_filename/1 - use utc_string/0 provided by couch_util.erl - use ddoc_cache:open/2 to open design document - extend test from couch_db to fabric Bugzid: 68276
jiangphcn
force-pushed
the
68276_new_purge_api
branch
from
2f3b67f to
f916079
Compare
2 participants
No description provided.