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

clientv3test: add comments for clientv3test #16920

Merged
merged 1 commit into from
Nov 23, 2023

Conversation

shaoqin2
Copy link
Contributor

Add tests for clientv3test.TestUnresolvableOrderViolation and clientv3test.TestEndpointSwitchResolvesViolation

Improve TestEndpointSwitchResolvesViolation with a partition resolved return no error case

Addresses #16865

@shaoqin2 shaoqin2 force-pushed the shlu/contribfest branch 3 times, most recently from 2f5c9ce to c684a1f Compare November 11, 2023 23:31
@@ -78,8 +81,16 @@ func TestEndpointSwitchResolvesViolation(t *testing.T) {
if err != ordering.ErrNoGreaterRev {
t.Fatal("While speaking to partitioned leader, we should get ErrNoGreaterRev error")
}

clus.Members[2].RecoverPartition(t, clus.Members[:2]...)
time.Sleep(1 * time.Second) // give enough time for the operation
Copy link
Member

Choose a reason for hiding this comment

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

NOTE: The orderingKv.Get doesn't have backoff try. It might make the test case flaky since the CI VM is unstable and the 1 second might be not enough for member[2] to get latest data.

Sorry, I don't have good idea to ensure the data is synced from leader. Maybe we can remove clientv3.WithSerializable() option to force to use linear read.

Copy link
Contributor Author

@shaoqin2 shaoqin2 Nov 14, 2023

Choose a reason for hiding this comment

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

I think when creating this orderingKv object, we passed the retry function NewOrderViolationSwitchEndpointClosure

	orderingKv := ordering.NewKV(cli.KV, ordering.NewOrderViolationSwitchEndpointClosure(cli))

when the Get fails, we will enter the retry based on

		err = kv.orderViolationFunc(op, r, prevRev)
		if err != nil {
			return nil, err
		}

Am I understanding this correctly? The only thing missing might be an actual backoff function, as it is I think it immediately retries, not sure if that's what you meant.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for unclear comment.

The orderingKv did retry immediately.

However, the Github Action CI VM is unstable. When you recover member[2] from network partition and sleep 1s, it doesn't guarantee the member[2] can sync with leader during ordering's retry.

The Line 87 _, err = orderingKv.Get(ctx, "foo", clientv3.WithSerializable()) still might return error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah that make sense. The idea is with default linearizable it will be forced to talk to the other members to ensure the most recent data is returned when Get is called right?

Removed with WithSerializable option from the call. Let me know if looks good

Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks!

ping @serathius @ahrtr to trigger the CI~

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks @shaoqin2

@ahrtr ahrtr merged commit 82fe457 into etcd-io:main Nov 23, 2023
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants