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.

Sign up for GitHub

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

Jump to bottom

feat(bigtable): run E2E test over DirectPath #3116

Merged
merged 7 commits into from Dec 4, 2020
Merged

feat(bigtable): run E2E test over DirectPath #3116

merged 7 commits into from Dec 4, 2020

Conversation

mohanli-ml
Copy link
Contributor

@mohanli-ml mohanli-ml commented Oct 30, 2020 •
edited

In the relevant GAX library PR googleapis/google-api-go-client#732, we removed environment variable GOOGLE_CLOUD_ENABLE_DIRECT_PATH, and expose an internal option to Yoshi library, so that Yoshi library can decide if it want to attempt DirectPath or not. In this PR,
we make use of this option to run E2E test over DirectPath. Relevant bug is b/172700854.

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 30, 2020
@mohanli-ml mohanli-ml mentioned this pull request Oct 30, 2020
Merged
@codyoss codyoss added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 30, 2020
@codyoss
Copy link
Member

codyoss commented Oct 30, 2020

Let's tackle the other PR first. This won't work until that is in, released and bumped the dependency here.

@tritone tritone requested a review from kolea2 October 30, 2020 14:21
tritone
tritone reviewed Oct 30, 2020
View reviewed changes
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

+1 to Cody's comment. Also FYI @kolea2

@mohanli-ml mohanli-ml changed the title Attempt DirectPath by default feat(bigtable): attempt DirectPath by default Oct 30, 2020
@mohanli-ml mohanli-ml force-pushed the dp-branch branch 2 times, most recently from ffe411b to b3ee65b Compare October 30, 2020 22:30
@product-auto-label product-auto-label bot added the api: bigtable Issues related to the Bigtable API. label Oct 31, 2020
@mohanli-ml mohanli-ml changed the title feat(bigtable): attempt DirectPath by default feat(bigtable): add an experimental attempt DirectPath flag Nov 5, 2020
@mohanli-ml mohanli-ml force-pushed the dp-branch branch 2 times, most recently from ebb00f7 to 687595b Compare November 5, 2020 23:42
@codyoss
Copy link
Member

codyoss commented Nov 6, 2020 •
edited

@mohanli-ml I did release a new version of google.golang.org/api if you want to bump the dependency here the code should now compile.

@mohanli-ml mohanli-ml changed the title feat(bigtable): add an experimental attempt DirectPath flag feat(bigtable): Run E2E test over DirectPath Nov 7, 2020
@mohanli-ml mohanli-ml requested review from igorbernstein2 and removed request for kolea2 November 7, 2020 02:59
@mohanli-ml mohanli-ml removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 7, 2020
@mohanli-ml mohanli-ml requested a review from a team as a code owner November 11, 2020 19:59
tbpg
tbpg suggested changes Nov 12, 2020
View reviewed changes
bigtable/bigtable.go Outdated
@@ -81,6 +83,13 @@ func NewClientWithConfig(ctx context.Context, project, instance string, config C
// TODO(grpc/grpc-go#1388) using connection pool without WithBlock
// can cause RPCs to fail randomly. We can delete this after the issue is fixed.
option.WithGRPCDialOption(grpc.WithBlock()))

// Expose an experimental flag to make client attempt DirectPath.
// Once e2e tests are finished, this flag will be removed, and client
Copy link
Contributor

Choose a reason for hiding this comment

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

What does "Once e2e tests are finished" mean? Seems like we should be able to add this option from the test directly, rather than through an environment variable?

As-is, this seems to be doing more than adding a test, given this is a non-test file. In general, we should avoid adding to the public API when we know we're going to remove it in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are going to release directpath for bigtable, and we designed some e2e test scenarios that needs to be covered before the release (See the background and what we have done for java-bigtable in go/directpath-e2e-test-scenarios). I replace the environment with a global variable, but the idea should be the same.

We need this option now because we do not want to rashly set internaloption.EnableDirectPath to true, which is what we will do after all e2e tests. A similar PR for previous work in java-bigtable can be found googleapis/java-bigtable#467

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I'm still confused. Can we avoid adding anything to the exported API? Can the test pass the internaloption.EnableDirectPath option when appropriate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Tyler, sorry I did not make myself clear. So DirectPath is a new feature for bigtable, and ultimately we will need to add internaloption.EnableDirectPath(true) to the library code to support this feature. For now, we are using a flag that is passed from tests in internaloption.EnableDirectPath() because we needs to test this feature first. But since ultimately we will change the library code, I think it is reasonable to add the internal option now. Does this make sense to you? Please let me know if you have more questions. 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.

I moved the internal option to test files, and the flag is removed.

bigtable/integration_test.go Show resolved Hide resolved
bigtable/integration_test.go Outdated Show resolved Hide resolved
@mohanli-ml mohanli-ml requested a review from tbpg November 12, 2020 19:12
@mohanli-ml mohanli-ml force-pushed the dp-branch branch 2 times, most recently from 4a99910 to 2b0d170 Compare November 23, 2020 17:43
tbpg
tbpg approved these changes Dec 3, 2020
View reviewed changes
bigtable/export_test.go Outdated
@@ -71,6 +77,7 @@ type IntegrationEnv interface {
NewInstanceAdminClient() (*InstanceAdminClient, error)
NewClient() (*Client, error)
Close()
GetPeer() *peer.Peer
Copy link
Contributor

Choose a reason for hiding this comment

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

This can just be called Peer. See https://golang.org/doc/effective_go.html#Getters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks!

@tbpg
Copy link
Contributor

tbpg commented Dec 3, 2020

Also, no need to force push. We'll squash into a single commit with the right message when merging.

@tbpg tbpg changed the title feat(bigtable): Run E2E test over DirectPath feat(bigtable): run E2E test over DirectPath Dec 4, 2020
@tbpg tbpg added the automerge Merge the pull request once unit tests and other checks pass. label Dec 4, 2020
@gcf-merge-on-green gcf-merge-on-green bot merged commit 948452c into googleapis:master Dec 4, 2020
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tritone tritone tritone approved these changes

@tbpg tbpg tbpg approved these changes

@kolea2 kolea2 kolea2 approved these changes

@igorbernstein2 igorbernstein2 Awaiting requested review from igorbernstein2

@codyoss codyoss Awaiting requested review from codyoss

Assignees
No one assigned
Labels
api: bigtable Issues related to the Bigtable API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mohanli-ml @codyoss @tbpg @tritone @kolea2