Skip to content

Conversation

@voidzcy
Copy link
Contributor

@voidzcy voidzcy commented Mar 3, 2021

I am trying to add a new API for exposing some unique ID that represents the Channel to its resolver. It could just be ManagedChannelImpl's logId (the long value).

We want this to be a required attribute for NameResolver.Args to avoid confusion. So deleting old APIs (NameResolver.Helper) without the necessity to add such an attribute for it as well.

@voidzcy voidzcy requested review from dapengzhang0 and ejona86 March 3, 2021 21:30
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

This looks fine, but I will have some questions about this unique id you are referring to and question if we'll make it required.

@dapengzhang0
Copy link
Contributor

but I will have some questions about this unique id you are referring to and question if we'll make it required

Background about the need of propagate channel id to resolver: #7922 (comment)

@voidzcy
Copy link
Contributor Author

voidzcy commented Mar 3, 2021

This looks fine, but I will have some questions about this unique id you are referring to and question if we'll make it required.

This is for one kind of affinity. We want RPCs sent through the same Channel have the same hash value. C-core is just using the memory address of the Channel.

Whether it should be required or optional in NameResolver.Args is identical to our previous discussion for other fields like ChannelLogger. We (ManagedChannelImpl) are the only creator of NameResolver.Args, we can definitely make every field required, if not consider cumbersomeness for testing. But a long field is easy to make up, the API users can easily use whatever value they want if this is something they do not care. Making it required gives better guarantee and reduces burden for API users.

@dapengzhang0
Copy link
Contributor

C-core is just using the memory address of the Channel.

I think c-core reuses the underlying channel for the same address.

@ejona86
Copy link
Member

ejona86 commented Mar 4, 2021

We want RPCs sent through the same Channel have the same hash value.

Why not just have the NameResolver choose a random value? Yes, it would be lost if the channel goes into IDLE mode, but I don't see how that's a problem in the slightest.

@dapengzhang0
Copy link
Contributor

Why not just have the NameResolver choose a random value? Yes, it would be lost if the channel goes into IDLE mode, but I don't see how that's a problem in the slightest.

The client needs some kind of affinity - What I'm concerning is if there's a long client session that the channel goes into IDLE and reactivated.

@ejona86
Copy link
Member

ejona86 commented Mar 4, 2021

The client needs some kind of affinity - What I'm concerning is if there's a long client session that the channel goes into IDLE and reactivated.

Idle mode takes 30 minutes to enter by default. I think it is safe to say any affinity is either lost or not worth much trying to preserve at that point, as the affinity in to reduce latency and there will be plenty of latency bringing the channel back online again. So even if someone chose to change the idleness to 5 minutes or 1 minute, I don't think I see much of a problem.

@voidzcy
Copy link
Contributor Author

voidzcy commented Mar 4, 2021

I commented on C-core's PR (as they are also just using the resolver's address in the draft) and got a similar answer. Yeah, I agree affinity may not worth much for RPCs after a period in the order of minutes.

We can still merge this PR, as it's irrelevant. So I would not need to plumb a unique ID for Channel to the resolver.

@ejona86
Copy link
Member

ejona86 commented Mar 4, 2021

Merging this PR SGTM. The APIs were deprecated two years ago, which is plenty of time to migrate.

@voidzcy voidzcy merged commit 37b94b3 into grpc:master Mar 4, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants