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 2024/03/16 07:13:24 UTC
[PR] Fix case clause error in replicator _scheduler/docs response [couchdb]
nickva opened a new pull request, #5008:
URL: https://github.com/apache/couchdb/pull/5008
When a job is completing, there is a period of time when the document isn't updated yet with the state, and it's also not present in the scheduler ets table. In that case we return `nil`, but we don't account for that case in the _scheduler/docs fabric calls.
To fix it, account for `nil` whenever we get the doc 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] Fix case clause error in replicator _scheduler/docs response [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #5008:
URL: https://github.com/apache/couchdb/pull/5008#discussion_r1527139130
##########
src/couch_replicator/test/eunit/couch_replicator_scheduler_docs_tests.erl:
##########
@@ -133,9 +133,6 @@ t_scheduler_docs_total_rows({_Ctx, {RepDb, Source, Target}}) ->
case req(get, SchedulerDocsUrl) of
{200, #{<<"docs">> := [_ | _]} = Decoded} ->
Decoded;
- {_, #{<<"error">> := Error, <<"reason">> := Reason}} ->
Review Comment:
I would not remove this clause. I understand that this PR fixes _one_ of the possible reasons for having things going wrong. But leaving this here could help in the future to get the necessary information right away. For example, when one wants to do something with the respective code and makes an unintentional mistake. Without this clause it becomes masked and shown as timeout, which is misleading.
--
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] Fix case clause error in replicator _scheduler/docs response [couchdb]
Posted by "pgj (via GitHub)" <gi...@apache.org>.
pgj commented on code in PR #5008:
URL: https://github.com/apache/couchdb/pull/5008#discussion_r1527471183
##########
src/couch_replicator/test/eunit/couch_replicator_scheduler_docs_tests.erl:
##########
@@ -133,9 +133,6 @@ t_scheduler_docs_total_rows({_Ctx, {RepDb, Source, Target}}) ->
case req(get, SchedulerDocsUrl) of
{200, #{<<"docs">> := [_ | _]} = Decoded} ->
Decoded;
- {_, #{<<"error">> := Error, <<"reason">> := Reason}} ->
Review Comment:
> If we get an error or anything else it should fail immediately with a bad match.
Yes, that is what #5007 is trying to implement. This focuses on intercepting errors in the response, explore the context, report it to the user (surface it), and then raise an error (make the test fail). But I can see that treating anything else than the happy path as error is probably a simpler way to express the same. Do you think that the resulting "bad match" in your case will give enough information for the developer?
> In general, or at least in Apache CouchDB, debug logs look a bit out of place. [..] Otherwise after a few years nobody knows why they are there or if they are still needed.
Well, with this debug logging of mine we found a bug already. And one could still "blame" that part of the code to learn about [the motivation](https://github.com/apache/couchdb/commit/d7188ba8dea81739e8535b92ede1c6613bb598f8) :-)
Either way it goes, I do not have a strong opinion about the related style or the habits. All I would consider meaningful is to avoid masking errors by timeouts.
--
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] Fix case clause error in replicator _scheduler/docs response [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5008:
URL: https://github.com/apache/couchdb/pull/5008#discussion_r1527412175
##########
src/couch_replicator/test/eunit/couch_replicator_scheduler_docs_tests.erl:
##########
@@ -133,9 +133,6 @@ t_scheduler_docs_total_rows({_Ctx, {RepDb, Source, Target}}) ->
case req(get, SchedulerDocsUrl) of
{200, #{<<"docs">> := [_ | _]} = Decoded} ->
Decoded;
- {_, #{<<"error">> := Error, <<"reason">> := Reason}} ->
Review Comment:
A better improvement here might to just be precise about what responses we expect. Instead of any map we should be explicit we wait while docs list is empty, and then return when we have some docs. If we get an error or anything else it should fail immediately with a bad match. I'll update it to do that.
In general, or at least in Apache CouchDB, debug logs look a bit out of place. We usually add them temporarily until we find the issue which is causing flakiness then remove them. Otherwise after a few years nobody knows why they are there or if they are still needed. Here is one example for instance I added 4 years ago: https://github.com/apache/couchdb/blob/b964a848d6ccf9494edc00466a19eed7ee6b44d1/src/mem3/src/mem3_sync_event_listener.erl#L314 no idea if it's still needed or useful at this point.
--
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] Fix case clause error in replicator _scheduler/docs response [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva commented on code in PR #5008:
URL: https://github.com/apache/couchdb/pull/5008#discussion_r1527564359
##########
src/couch_replicator/test/eunit/couch_replicator_scheduler_docs_tests.erl:
##########
@@ -133,9 +133,6 @@ t_scheduler_docs_total_rows({_Ctx, {RepDb, Source, Target}}) ->
case req(get, SchedulerDocsUrl) of
{200, #{<<"docs">> := [_ | _]} = Decoded} ->
Decoded;
- {_, #{<<"error">> := Error, <<"reason">> := Reason}} ->
Review Comment:
The typical pattern is to assert only cases we expect and then it should crash if it gets something else. The code didn't do that before, it accepted any map and continued to wait.
> Well, with this debug logging of mine we found a bug already.
That was very nice, good stuff!
--
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] Fix case clause error in replicator _scheduler/docs response [couchdb]
Posted by "nickva (via GitHub)" <gi...@apache.org>.
nickva merged PR #5008:
URL: https://github.com/apache/couchdb/pull/5008
--
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