[Test] Fix CRUDDocumentationIT #28457
Conversation
nik9000
left a comment
nik9000
left a comment
There was a problem hiding this comment.
I expect assertBusy will produce better error messages one failure. Also, I don't think you had to move these to the last ones, right? I don't mind either way, but just for my own sanity, can you confirm that or correct me?
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
LGTM I would do assertBusy too, I am curious about the answers to Nik's questions too.
There was a problem hiding this comment.
what happens if we don't wait for this one?
There was a problem hiding this comment.
I was also thinking, given that this is a recurring problem and we don't want to have latches to show in our docs, shall we make this more clear doing something through a listener that blocks (without showing in the snippets what the listener really does)?
There was a problem hiding this comment.
what happens if we don't wait for this one?
After each test method, the ESRestTestCase.cleanUpCluster() is executed. As you know this method deletes the cluster indices, waits for the pending cluster states tasks to finish and logs a message if there is still inflight requests.
By not waiting for the async request to respond, it's possible that an index got re created because the index (or update) operation is processed after the indices have been deleted and s recreates the index and breaks following tests.
I'm pretty sure I saw this error, but sadly I can't find a test execution log that clearly shows this behavior. In any way I think it's better to wait for the completion of any write operation that could recreate an index for the reason I mentioned.
There was a problem hiding this comment.
makes sense, thanks for explaining.
There was a problem hiding this comment.
what do you think about the listener idea above?
There was a problem hiding this comment.
This will have an impact on documentation (2 code snippets instead of one), should we align all documentation for async execution to be like this? I think so.
There was a problem hiding this comment.
yes I would adapt the docs, let me know if you prefer me to take this over given that it's becoming a bigger task.
There was a problem hiding this comment.
I can do it. Do you prefer a follow up PR or be part of this one?
There was a problem hiding this comment.
once you have changed the id of the update, it won't sneak in anymore and cause version conflict, so I think it doesn't necessarily needs to be moved down? But maybe it's good like you did. Maybe we don't even need to wait for it at this point?
There was a problem hiding this comment.
That was for readability, as some test use the same document to index or update multiple times. But I can move them back if you prefer.
Maybe we don't even need to wait for it at this point?
We need to wait for any writing operation, but we can relax this for read operations.
There was a problem hiding this comment.
same comment as above, once we change the id, do we need to still wait for it?
|
Thanks for the reviews @nik9000 and @javanna.
This is not a requirement of course but some tests use the same document (like /posts/doc/1) and update or index it multiple times. Since I changed the async operations to use a different doc I moved them down so that it does not interleave with other request executions as I found this more readable. |
Similarly to other documentation tests in the high level client, the asynchronous operation like update, index or delete can make the test fail if it sneak in between the middle of another operation. This commit moves the async doc tests to be the last ones executed and adds await busy loops to ensure that the asynchronous operations are correctly terminated. closes elastic#28446
Similarly to other documentation tests in the high level client, the asynchronous operation like update, index or delete can make the test fail if it sneak in between the middle of another operation. This commit moves the async doc tests to be the last ones executed and adds assert busy loops to ensure that the asynchronous operations are correctly terminated. closes #28446
This commit splits the async execution documentation into 2 parts, one for the async method itself and one for the action listener. This allows to add more doc and to use CountDownLatches in doc tests to wait for asynchronous operations to be completed before moving to the next test. It also renames few files. Related to elastic#28457
This commit splits the async execution documentation into 2 parts, one for the async method itself and one for the action listener. This allows to add more doc and to use CountDownLatches in doc tests to wait for asynchronous operations to be completed before moving to the next test. It also renames few files. Related to #28457
This commit splits the async execution documentation into 2 parts, one for the async method itself and one for the action listener. This allows to add more doc and to use CountDownLatches in doc tests to wait for asynchronous operations to be completed before moving to the next test. It also renames few files. Related to #28457
Similarly to other documentation tests in the high level client, the asynchronous operation like update, index or delete can make the test fail if it sneak in between the middle of another operation. This commit moves the async doc tests to be the last ones executed and adds assert busy loops to ensure that the asynchronous operations are correctly terminated. closes #28446
This commit splits the async execution documentation into 2 parts, one for the async method itself and one for the action listener. This allows to add more doc and to use CountDownLatches in doc tests to wait for asynchronous operations to be completed before moving to the next test. It also renames few files. Related to #28457
4 participants
Similarly to other documentation tests in the high level client, the
asynchronous operation like update, index or delete can make the test
fail if it sneak in between the middle of another operation.
This commit moves the async doc tests to be the last ones executed and
adds await busy loops to ensure that the asynchronous operations are
correctly terminated.
closes #28446