Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test_reconfigure_domains.py: Improve comments and readability #537

Merged
merged 2 commits into from
Dec 11, 2024

Conversation

kaikulimu
Copy link
Collaborator

Subtle improvements, and use strong consistency domain instead of eventual consistency

@kaikulimu kaikulimu requested a review from a team as a code owner December 5, 2024 02:13
@kaikulimu kaikulimu force-pushed the test_reconfigure_domains_py branch from 5e9f987 to 818696c Compare December 5, 2024 02:18
Copy link

@bmq-oss-ci bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build 396 of commit 818696c has completed with FAILURE

@678098 678098 self-assigned this Dec 11, 2024
@678098 678098 self-requested a review December 11, 2024 08:03
Copy link
Collaborator

@678098 678098 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments

URI_PRIORITY_1 = f"bmq://{tc.DOMAIN_PRIORITY}/abcd-queue"
URI_PRIORITY_2 = f"bmq://{tc.DOMAIN_PRIORITY}/qrst-queue"
URI_PRIORITY_1 = f"bmq://{tc.DOMAIN_PRIORITY_SC}/abcd-queue"
URI_PRIORITY_2 = f"bmq://{tc.DOMAIN_PRIORITY_SC}/qrst-queue"
Copy link
Collaborator

@678098 678098 Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want to test non-SC domain here anymore?
I am thinking if we can parametrize this with fixtures.
I am also ok if we just switch to SC here without extra work, or we can add an issue to support non-SC if it's worth it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem of parameterize testing non-SC and SC domains exist for many integration test files. Will create an issue to support all of them

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a significant difference in the broker logics so we are better to support both? If we don't expect differences in the logics, might just leave SC-one

Copy link
Collaborator Author

@kaikulimu kaikulimu Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is SC domains are more asynchronous as they need to wait for quorum of receipts from replicas, so it's more susceptible to race conditions. I will create an issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/integration-tests/test_reconfigure_domains.py Outdated Show resolved Hide resolved
src/integration-tests/test_reconfigure_domains.py Outdated Show resolved Hide resolved
src/integration-tests/test_reconfigure_domains.py Outdated Show resolved Hide resolved
src/integration-tests/test_reconfigure_domains.py Outdated Show resolved Hide resolved
Signed-off-by: Yuan Jing Vincent Yan <[email protected]>
@kaikulimu kaikulimu force-pushed the test_reconfigure_domains_py branch from 74b6eb4 to a61b854 Compare December 11, 2024 09:16
@kaikulimu
Copy link
Collaborator Author

@678098 Back to you

Copy link
Collaborator

@678098 678098 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@678098 678098 assigned kaikulimu and unassigned 678098 Dec 11, 2024
@kaikulimu kaikulimu merged commit 35cf678 into bloomberg:main Dec 11, 2024
28 of 30 checks passed
@kaikulimu kaikulimu deleted the test_reconfigure_domains_py branch December 11, 2024 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants