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

grpc-js: Add support for grpc.service_config_disable_resolution #2277

Merged
merged 1 commit into from Jan 3, 2023
Merged

grpc-js: Add support for grpc.service_config_disable_resolution #2277

merged 1 commit into from Jan 3, 2023

Conversation

kleinsch
Copy link
Contributor

@kleinsch kleinsch commented Nov 16, 2022 •
edited

Adds support for grpc.service_config_disable_resolution to fix #1518 .

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 16, 2022 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: kleinsch / name: Nick Kleinschmidt (7f95346)

@kleinsch
Copy link
Contributor Author

/easycla

murgatroid99
murgatroid99 approved these changes Nov 16, 2022
View reviewed changes
Copy link
Member

@murgatroid99 murgatroid99 left a comment

Choose a reason for hiding this comment

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

Looks good with a couple of minor changes.

packages/grpc-js/src/channel-options.ts
@@ -47,6 +47,7 @@ export interface ChannelOptions {
'grpc.max_connection_age_ms'?: number;
'grpc.max_connection_age_grace_ms'?: number;
'grpc-node.max_session_memory'?: number;
'grpc.service_config_disable_resolution'?: number;
Copy link
Member

Choose a reason for hiding this comment

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

Please also add this to the recognizedOptions object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

packages/grpc-js/src/resolver-dns.ts Outdated
@@ -255,7 +260,7 @@ class DnsResolver implements Resolver {
);
/* If there already is a still-pending TXT resolution, we can just use
* that result when it comes in */
if (this.pendingTxtPromise === null) {
if (this.pendingTxtPromise === null && this.isServiceConfigEnabled) {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to switch the order of the checks in this if statement. It makes more sense to check whether the behavior is enabled before checking whether the state is right to take that action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@kleinsch kleinsch force-pushed the add_service_config_disable_resolution branch 3 times, most recently from 94c8779 to 7f95346 Compare December 8, 2022 17:09
@kleinsch
Copy link
Contributor Author

kleinsch commented Dec 8, 2022 •
edited

@murgatroid99 I addressed feedback and added a test for this that hits a domain I own. I'm OK leaving that domain up for this test to work. Or could skip() the test if you don't want the dependency? Or you could set up a TXT record on unittest.grpc.io? Let me know

@murgatroid99
Copy link
Member

Well, the first test is failing, so that should be fixed first. Once the tests pass, I think it would be best to skip them, to avoid a dependency on an external domain.

@kleinsch
Copy link
Contributor Author

kleinsch commented Dec 8, 2022

Sounds good. They're passing locally but failing in CI with timeouts. If I can't get them working I'll just remove.

@murgatroid99 murgatroid99 changed the base branch from master to @grpc/grpc-js@1.8.x December 8, 2022 21:13
@murgatroid99
Copy link
Member

Currently it doesn't indicate if it's timing out because it's actually slow, or because it can't retrieve the TXT record at all. Unfortunately, I don't think Node's DNS API makes that distinction either, which is why we split up IP address and TXT record requests in the first place.

If it's passing locally I'll say that's good enough for me, so let's proceed as though the tests passed, and skip them.

@kleinsch kleinsch force-pushed the add_service_config_disable_resolution branch 2 times, most recently from 42bf036 to c930976 Compare December 8, 2022 22:07
@kleinsch
Copy link
Contributor Author

kleinsch commented Dec 8, 2022

@murgatroid99 Got the test working and changed to it.skip so should be good to go.

murgatroid99
murgatroid99 reviewed Dec 16, 2022
View reviewed changes
packages/grpc-js/test/test-resolver.ts Outdated
done();
}, 3_000);
}
).timeout(10_000);
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is causing an error now that the test is skipped. I think you can avoid that by using a non-arrow function for the test behavior and calling this.timeout(10_000).

@kleinsch kleinsch force-pushed the add_service_config_disable_resolution branch from c930976 to 8e5a484 Compare December 17, 2022 22:15
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 17, 2022 •
edited

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: kleinsch / name: Nick Kleinschmidt (b240ca1)

@kleinsch kleinsch force-pushed the add_service_config_disable_resolution branch from 8e5a484 to b240ca1 Compare December 17, 2022 22:18
@kleinsch kleinsch force-pushed the add_service_config_disable_resolution branch from b240ca1 to 677c009 Compare December 17, 2022 22:19
@kleinsch
Copy link
Contributor Author

I didn't realize ./run-tests.sh isn't the same as cd packages/grpc-js; npm test. Should be fixed.

@murgatroid99 murgatroid99 merged commit 08cc571 into grpc:@grpc/grpc-js@1.8.x Jan 3, 2023
@murgatroid99 murgatroid99 mentioned this pull request Jan 3, 2023
Merged
@murgatroid99
Copy link
Member

This has been published in version 1.8.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@murgatroid99 murgatroid99 murgatroid99 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for "grpc.service_config_disable_resolution" option in grpc-js
3 participants
@kleinsch @murgatroid99 @kokoro-team