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

RATIS-1125. Fix TestDataStream #247

Merged
merged 2 commits into from Nov 2, 2020
Merged

Conversation

amaliujia
Copy link
Contributor

What changes were proposed in this pull request?

TestDataStream no long tests right state machine after RATIS-1124. It is because RaftServer uses its own map to keep state machine so the original state machines used to test server's writes no long work.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/RATIS-1125

How was this patch tested?

UT

@amaliujia
Copy link
Contributor Author

R: @szetszwo @runzhiwang

@amaliujia amaliujia changed the title RATIS.1125. Fix TestDataStream RATIS-1125. Fix TestDataStream Nov 1, 2020
@amaliujia amaliujia closed this Nov 1, 2020
@amaliujia amaliujia reopened this Nov 1, 2020
@runzhiwang runzhiwang closed this Nov 2, 2020
@runzhiwang runzhiwang reopened this Nov 2, 2020
Copy link
Contributor

@runzhiwang runzhiwang left a comment

Choose a reason for hiding this comment

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

LGTM

@runzhiwang runzhiwang closed this Nov 2, 2020
@runzhiwang runzhiwang reopened this Nov 2, 2020
Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

+1 thanks for fixing it.

@runzhiwang runzhiwang closed this Nov 2, 2020
@runzhiwang runzhiwang reopened this Nov 2, 2020
@runzhiwang runzhiwang merged commit e0958c9 into apache:master Nov 2, 2020
@runzhiwang
Copy link
Contributor

@amaliujia Thanks the patch. @szetszwo Thanks for review. I have merged it.

@szetszwo
Copy link
Contributor

szetszwo commented Nov 2, 2020

Oops, just find that the fix does not work since there are multiple servers. Each server should have its own map. Let me fix it in RATIS-1127.

@amaliujia amaliujia deleted the RATIS-1125 branch November 2, 2020 02:35
@amaliujia
Copy link
Contributor Author

@szetszwo

hmm I have thought about that. For current test setup it is ok. Because the data structure is ConcurrentMap<RaftGroupId, MultiDataStreamStateMachine> and currently each client uses a random group id, so sharing the same map among servers does not overwrite each other thus it is still working.

For the multiple group change, I will need to update this test because in that change, servers will share the same group id so they cannot share the same map anymore.

@szetszwo
Copy link
Contributor

szetszwo commented Nov 2, 2020

... Because the data structure is ConcurrentMap<RaftGroupId, MultiDataStreamStateMachine> and currently each client uses a random group id ...

I see. It would work but it is confusing. Please put some comments on the code for work around like this. :)

symious pushed a commit to symious/ratis that referenced this pull request Feb 7, 2024
* RATIS.1125. Fix TestDataStream

* fixup! add comment back
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants