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

Lro method settings #779

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

jskeet
Copy link
Collaborator

@jskeet jskeet commented Apr 4, 2024

Fixes #557.

I'm going to run this locally against all APIs to see whether we get any changes - I don't expect there to be any settings configured in MethodSettings yet. (They're all in _gapic.yaml files.)

We should then discuss what we want to do.

@jskeet jskeet added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 4, 2024
@jskeet jskeet requested a review from a team as a code owner April 4, 2024 17:19
Copy link
Contributor

@amanda-tarafa amanda-tarafa left a comment

Choose a reason for hiding this comment

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

Yep, this looks good.

@@ -1,4 +1,4 @@
// Copyright 2019 Google LLC
// Copyright 2019 Google LLC
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: undo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could do - this was just the result of copying the generated file into place :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah OK then, don't, else it will always show up.

@@ -27,6 +27,15 @@ service Lro {
};
option (google.api.method_signature) = "name";
}

// Test an LRO RPC with customized default polling settings.
rpc CustomDefaultPollingMethod(Request) returns(google.longrunning.Operation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is just a test method, but the combination of custom and default is always very confusing to me. Can you use default override instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or maybe PublishingSettingsPollingMethod? ServiceConfigPollingMethod? Happy to bike shed this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, any one of those is fine as well. Whatever you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny ping on this one.

@@ -1,4 +1,4 @@
// Copyright 2019 Google LLC
// Copyright 2019 Google LLC
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah OK then, don't, else it will always show up.

@@ -27,6 +27,15 @@ service Lro {
};
option (google.api.method_signature) = "name";
}

// Test an LRO RPC with customized default polling settings.
rpc CustomDefaultPollingMethod(Request) returns(google.longrunning.Operation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, any one of those is fine as well. Whatever you prefer.

@jskeet
Copy link
Collaborator Author

jskeet commented Apr 4, 2024

Generation has completed - it would lead to no changes in google-cloud-dotnet.

@jskeet
Copy link
Collaborator Author

jskeet commented May 22, 2024

I'm going to pull the first commit from here into a separate PR so we can keep this PR as "just the new feature".

This currently just uses the default poll settings, so shouldn't affect the generated code at all.
(This is the first step to using the settings from the service config.)
(This is specified in publishing settings, but feels like it's more LRO-oriented than settings-oriented.)
…RO method

(This is to make the diff for "with the feature" simpler to understand.)
(This includes the change in generated code, as it's so small.)
@@ -27,6 +27,15 @@ service Lro {
};
option (google.api.method_signature) = "name";
}

// Test an LRO RPC with customized default polling settings.
rpc CustomDefaultPollingMethod(Request) returns(google.longrunning.Operation) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny ping on this one.

@jskeet
Copy link
Collaborator Author

jskeet commented May 22, 2024

Will have a look tomorrow. I'm not actually in a hurry to get this in, but I wanted it in a better shape :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make LRO polling generation configurable
2 participants