-
Notifications
You must be signed in to change notification settings - Fork 636
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
grpc-js: Add support for grpc.service_config_disable_resolution #2277
grpc-js: Add support for grpc.service_config_disable_resolution #2277
Conversation
|
/easycla |
There was a problem hiding this 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.
@@ -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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
94c8779
to
7f95346
Compare
@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 |
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. |
Sounds good. They're passing locally but failing in CI with timeouts. If I can't get them working I'll just remove. |
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. |
42bf036
to
c930976
Compare
@murgatroid99 Got the test working and changed to |
done(); | ||
}, 3_000); | ||
} | ||
).timeout(10_000); |
There was a problem hiding this comment.
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)
.
c930976
to
8e5a484
Compare
|
8e5a484
to
b240ca1
Compare
b240ca1
to
677c009
Compare
I didn't realize |
This has been published in version 1.8.1. |
Adds support for
grpc.service_config_disable_resolution
to fix #1518 .