Fix flakiness of tests #1150
Fix flakiness of tests #1150
Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
| producer.flush() | ||
| msg = future.result() | ||
|
|
||
| schema_reader._offset_watcher.wait_for_offset(msg.offset(), timeout=5) |
There was a problem hiding this comment.
This is a blocking call. If offset is marked as seen the schema must be in the in memory database.
Can you enable the strict mode in the config for the test and see if there is an exception catched when handing the message?
Option is also to see if the timeout is elapsed, the call returns the boolean if condition was met. And on timeout the return value is False. That can be asserted.
There was a problem hiding this comment.
It was hard to get any logs.
Enabling strict mode, did not trigger any exception.
I thought we needed some timeouts when getting future. Added. wdyt
muralibasani
force-pushed
the
fix-flaky-schema-delete
branch
2 times, most recently
from
0d380db to
19f8508
Compare
muralibasani
force-pushed
the
fix-flaky-schema-delete
branch
from
d9240e3 to
daec2e2
Compare
muralibasani
force-pushed
the
fix-flaky-schema-delete
branch
from
daec2e2 to
97695a4
Compare
| msg = future.result() | ||
|
|
||
| schema_reader._offset_watcher.wait_for_offset(msg.offset(), timeout=5) | ||
| msg = future.result(timeout=2) |
There was a problem hiding this comment.
Please add some justification for this to the commit message, I have no idea why we would add a timeout here, as we are flushing right before.
There was a problem hiding this comment.
Well, don't have a hard reason for this , but generally I think it is resource intensive sometimes.
And timeout on the future object is provided by the api to handle such cases where processes are slow.
| ) | ||
| producer.flush() | ||
| msg = future.result() | ||
| msg = future.result(timeout=2) |
There was a problem hiding this comment.
Same here, we have flushed, waiting should not change anything. Have we actually observed something that would hint at this solving a problem we have? (I.e., I think this would raise if the future is not complete when calling .result(). Have we seen such exceptions?)
| config.admin_metadata_max_age = 2 | ||
| config.group_id = group_id | ||
| config.topic_name = topic_name | ||
| config.producer_acks = 1 |
There was a problem hiding this comment.
Please add reasoning in commit message, why this change?
| msg = future.result() | ||
| msg = future.result(timeout=2) |
| schema_reader._offset_watcher.wait_for_offset(msg.offset(), timeout=5) | ||
|
|
||
| seen = schema_reader._offset_watcher.wait_for_offset(msg.offset(), timeout=5) | ||
| assert seen is True |
There was a problem hiding this comment.
Why do we stop asserting here?
About this change - What it does
For the below tests
test_regression_config_for_inexisting_object_should_not_throw
test_regression_soft_delete_schemas_should_be_registered
References: #xxxxx
Why this way