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/05 12:40:31 UTC
[GitHub] [couchdb] bessbd opened a new pull request #3404: Fix and re-enable ChangesAsyncTest
bessbd opened a new pull request #3404:
URL: https://github.com/apache/couchdb/pull/3404
## Overview
The `ChangesAsyncTest` tests were broken and disabled. This PR fixes and re-enables them.
## Testing recommendations
`mix test --trace test/elixir/test/changes_async_test.exs`
## Related Issues or Pull Requests
<!-- If your changes affects multiple components in different
repositories please put links to those issues or pull requests here. -->
## Checklist
- [ ] Code is written and works correctly
- [ ] Changes are covered by tests
- [ ] 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
[GitHub] [couchdb] jjrodrig commented on a change in pull request #3404: Fix and re-enable ChangesAsyncTest
Posted by GitBox <gi...@apache.org>.
jjrodrig commented on a change in pull request #3404:
URL: https://github.com/apache/couchdb/pull/3404#discussion_r592626747
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -222,9 +222,13 @@ changes_callback(waiting_for_updates, Acc) ->
mochi = Resp1,
chunks_sent = ChunksSent + 1
}};
-changes_callback({timeout, _ResponseType}, Acc) ->
+changes_callback({timeout, ResponseType}, Acc) ->
#cacc{mochi = Resp, chunks_sent = ChunksSent} = Acc,
- {ok, Resp1} = chttpd:send_delayed_chunk(Resp, "\n"),
+ Chunk = case ResponseType of
+ "eventsource" -> "event: heartbeat\ndata: \n\n";
Review comment:
Sorry for the delay. I've been checking this and I think we are leaving dead code during the move to fdb. I suppose this should be addressed in some moment.
I did some checks changing the callback params to match with the old code [here](https://github.com/apache/couchdb/blob/82bd4d450711b0ff62616229851e5d19fd20e42f/src/chttpd/src/chttpd_changes.erl#L392) and it seems to work with some side effects in the auth cache changes monitor that can be easily solved.
The old timeout callbacks seem to be dead code, but with the test suite partially I'm not sure.
Your patch solves the issue in the new execution path, so we will go ahead.
----------------------------------------------------------------
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] bessbd merged pull request #3404: Fix and re-enable ChangesAsyncTest
Posted by GitBox <gi...@apache.org>.
bessbd merged pull request #3404:
URL: https://github.com/apache/couchdb/pull/3404
----------------------------------------------------------------
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] bessbd commented on a change in pull request #3404: Fix and re-enable ChangesAsyncTest
Posted by GitBox <gi...@apache.org>.
bessbd commented on a change in pull request #3404:
URL: https://github.com/apache/couchdb/pull/3404#discussion_r589088462
##########
File path: test/elixir/test/changes_async_test.exs
##########
@@ -51,13 +50,13 @@ defmodule ChangesAsyncTest do
create_doc_bar(db_name, "bar")
- {changes_length, last_seq_prefix} =
+ changes =
req_id.id
|> process_response(&parse_chunk/1)
- |> parse_changes_response()
- assert changes_length == 1, "should return one change"
- assert last_seq_prefix == "2-", "seq must start with 2-"
+ assert length(changes["results"]) == 1, "should return one change"
+ next_last_seq = changes["last_seq"]
+ assert next_last_seq > last_seq
Review comment:
I think they were comparable previously, too. Now the only thing about them is that they are monotonously increasing.
+CC @kocolosk
----------------------------------------------------------------
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] bessbd edited a comment on pull request #3404: Fix and re-enable ChangesAsyncTest
Posted by GitBox <gi...@apache.org>.
bessbd edited a comment on pull request #3404:
URL: https://github.com/apache/couchdb/pull/3404#issuecomment-792353477
Thank you for the review, @jjrodrig . Great comments! Please let me know if I've missed anything.
----------------------------------------------------------------
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] jjrodrig commented on a change in pull request #3404: Fix and re-enable ChangesAsyncTest
Posted by GitBox <gi...@apache.org>.
jjrodrig commented on a change in pull request #3404:
URL: https://github.com/apache/couchdb/pull/3404#discussion_r589060649
##########
File path: test/elixir/test/changes_async_test.exs
##########
@@ -34,12 +34,11 @@ defmodule ChangesAsyncTest do
)
changes = process_response(req_id.id, &parse_chunk/1)
- {changes_length, last_seq_prefix} = parse_changes_response(changes)
- assert changes_length == 1, "db should not be empty"
- assert last_seq_prefix == "1-", "seq must start with 1-"
Review comment:
It is the first time I've seen a 4.x change seq. It is completely different from the previous format. I see that we can not check anything about the seq.
##########
File path: test/elixir/lib/couch.ex
##########
@@ -80,6 +80,10 @@ defmodule Couch do
@request_timeout 60_000
@inactivity_timeout 55_000
+ def base_url() do
Review comment:
Nice, I should have done this during the porting.
##########
File path: test/elixir/test/changes_async_test.exs
##########
@@ -51,13 +50,13 @@ defmodule ChangesAsyncTest do
create_doc_bar(db_name, "bar")
- {changes_length, last_seq_prefix} =
+ changes =
req_id.id
|> process_response(&parse_chunk/1)
- |> parse_changes_response()
- assert changes_length == 1, "should return one change"
- assert last_seq_prefix == "2-", "seq must start with 2-"
+ assert length(changes["results"]) == 1, "should return one change"
+ next_last_seq = changes["last_seq"]
+ assert next_last_seq > last_seq
Review comment:
Now seqs are comparable?
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -222,9 +222,13 @@ changes_callback(waiting_for_updates, Acc) ->
mochi = Resp1,
chunks_sent = ChunksSent + 1
}};
-changes_callback({timeout, _ResponseType}, Acc) ->
+changes_callback({timeout, ResponseType}, Acc) ->
#cacc{mochi = Resp, chunks_sent = ChunksSent} = Acc,
- {ok, Resp1} = chttpd:send_delayed_chunk(Resp, "\n"),
+ Chunk = case ResponseType of
+ "eventsource" -> "event: heartbeat\ndata: \n\n";
Review comment:
Why is this required here? Previously, this seems to be managed at line [171](https://github.com/apache/couchdb/blob/db6a7822b9c5bc501bb20acb6739a1c0cf7f929c/src/chttpd/src/chttpd_db.erl#L171). We are addressing the root issue here or we are duplicating some logic?
----------------------------------------------------------------
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] bessbd commented on a change in pull request #3404: Fix and re-enable ChangesAsyncTest
Posted by GitBox <gi...@apache.org>.
bessbd commented on a change in pull request #3404:
URL: https://github.com/apache/couchdb/pull/3404#discussion_r589088249
##########
File path: src/chttpd/src/chttpd_db.erl
##########
@@ -222,9 +222,13 @@ changes_callback(waiting_for_updates, Acc) ->
mochi = Resp1,
chunks_sent = ChunksSent + 1
}};
-changes_callback({timeout, _ResponseType}, Acc) ->
+changes_callback({timeout, ResponseType}, Acc) ->
#cacc{mochi = Resp, chunks_sent = ChunksSent} = Acc,
- {ok, Resp1} = chttpd:send_delayed_chunk(Resp, "\n"),
+ Chunk = case ResponseType of
+ "eventsource" -> "event: heartbeat\ndata: \n\n";
Review comment:
The thing is, the server was sending plain `\n`s as heartbeat (which is not what is expected for eventsource). You can try it yourself with and without the change. A `GET` `/<db>/_changes?feed=eventsource&heartbeat=1000` should help reproducing the issue.
We may want to remove the lines you've mentioned above, but I don't know the codebase well enough to tell if that would break anything.
I'm hoping I'm not duplicating logic here, but I'm not 100% the previous related code isn't dead code.
IIRC https://github.com/apache/couchdb/pull/3404/files#diff-0ad4fb5d609221dac3b3f5faa48e061bd84f0745ecb0b6953b820d117b55cf5cR106 was the test case that was broken for this.
----------------------------------------------------------------
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] bessbd commented on pull request #3404: Fix and re-enable ChangesAsyncTest
Posted by GitBox <gi...@apache.org>.
bessbd commented on pull request #3404:
URL: https://github.com/apache/couchdb/pull/3404#issuecomment-796554611
Hi @jjrodrig ,
Can you please do a second / final pass on 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] bessbd commented on a change in pull request #3404: Fix and re-enable ChangesAsyncTest
Posted by GitBox <gi...@apache.org>.
bessbd commented on a change in pull request #3404:
URL: https://github.com/apache/couchdb/pull/3404#discussion_r589088281
##########
File path: test/elixir/lib/couch.ex
##########
@@ -80,6 +80,10 @@ defmodule Couch do
@request_timeout 60_000
@inactivity_timeout 55_000
+ def base_url() do
Review comment:
Thank you!
----------------------------------------------------------------
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] bessbd commented on pull request #3404: Fix and re-enable ChangesAsyncTest
Posted by GitBox <gi...@apache.org>.
bessbd commented on pull request #3404:
URL: https://github.com/apache/couchdb/pull/3404#issuecomment-797096716
Thank you for the review, @jjrodrig ! I'm about to merge this.
----------------------------------------------------------------
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] bessbd commented on a change in pull request #3404: Fix and re-enable ChangesAsyncTest
Posted by GitBox <gi...@apache.org>.
bessbd commented on a change in pull request #3404:
URL: https://github.com/apache/couchdb/pull/3404#discussion_r589088361
##########
File path: test/elixir/test/changes_async_test.exs
##########
@@ -34,12 +34,11 @@ defmodule ChangesAsyncTest do
)
changes = process_response(req_id.id, &parse_chunk/1)
- {changes_length, last_seq_prefix} = parse_changes_response(changes)
- assert changes_length == 1, "db should not be empty"
- assert last_seq_prefix == "1-", "seq must start with 1-"
Review comment:
The format of the `seq`s is different now. They no longer have the predictability they had in previous releases.
----------------------------------------------------------------
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] bessbd commented on pull request #3404: Fix and re-enable ChangesAsyncTest
Posted by GitBox <gi...@apache.org>.
bessbd commented on pull request #3404:
URL: https://github.com/apache/couchdb/pull/3404#issuecomment-792353477
Thank you for the review, @jjrodrig . Great comments. Please let me know if I've missed anything.
----------------------------------------------------------------
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