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

internal/grpcsync: move CallbackSerializer from xdsclient/internal to here #6153

Merged
merged 4 commits into from
Mar 31, 2023

Conversation

my4-dev
Copy link
Contributor

@my4-dev my4-dev commented Mar 25, 2023

Move CallbackSerializer from xdsclient/internal to grpcsync package
Please refer to this conversation in #6036 .

RELEASE NOTES: none

@@ -28,11 +28,17 @@ import (
"github.com/google/go-cmp/cmp"
)

const (
defaultTestWatchExpiryTimeout = 100 * time.Millisecond
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this const being anywhere?

Copy link
Contributor Author

@my4-dev my4-dev Mar 28, 2023

Choose a reason for hiding this comment

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

This const was originally defined in event_test.go at xdsclient package and it is used in callback_serializer_test.go.
Since const values with the same meaning were not defined, I've copied it here and resolved a compile error.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no file named event_test.go in the xdsclient package. Also, I checked callback_serializer_test.go and there is no usage of defaultTestWatchExpiryTimeout. Can you please delete it. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test entry point like the following is defined in event_test.go in the grpcsync package.
So callback_serializer_test.go depends on event_test.go. Should I revise this file name or do anything else?

type s struct {
	grpctest.Tester
}

func Test(t *testing.T) {
	grpctest.RunSubTests(t, s{})
}

Also, I didn't notice that there is no usage of defaultTestWatchExpiryTimeout. I've deleted this definition from callback_serializer_test.go.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I didn't notice that there is no usage of defaultTestWatchExpiryTimeout. I've deleted this definition from callback_serializer_test.go.

Yes, that's all I wanted. Thanks.

@easwars easwars changed the title Move CallbackSerializer from xdsclient/internal to grpcsync package internal/grpcsync: move CallbackSerializer from xdsclient/internal to here Mar 27, 2023
@easwars easwars added the Type: Internal Cleanup Refactors, etc label Mar 27, 2023
@easwars easwars added this to the 1.55 Release milestone Mar 27, 2023
@my4-dev
Copy link
Contributor Author

my4-dev commented Mar 31, 2023

Again, one test pipeline failed. A test time out seems to cause this fail.
I've implemented the following commands in my local machine and they've succeeded.

VET_SKIP_PROTO=1 ./vet.sh
go test -cpu 1,4 -timeout 7m ./...
go test -race -cpu 1,4 -timeout 7m ./...

May it be a flaky test?
I'm not sure why this fail occured.

@dfawley
Copy link
Member

dfawley commented Mar 31, 2023

This is a known flake: #6075

@easwars easwars merged commit e979919 into grpc:master Mar 31, 2023
1 check passed
@my4-dev my4-dev deleted the callbackSerializer_move_to_grpcsync branch April 1, 2023 05:55
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants