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

Permit binding failures to bypass logging #14

Open
jskeet opened this issue Sep 4, 2018 · 3 comments
Open

Permit binding failures to bypass logging #14

jskeet opened this issue Sep 4, 2018 · 3 comments

Comments

@jskeet
Copy link
Member

jskeet commented Sep 4, 2018

Currently there's a TODO to log a failure if a request looks like it should be bound via a key, but we don't have that key in the dictionary.

I know of at least one situation where that would be problematic: in Spanner, most requests on the same session should use the same gRPC channel that created the session... but partitioned reads don't have this requirement. It's expected just for this case that a single session ID is used on multiple machines or processes. In this particular at least this can be detected by the presence of a partition ID in the request, but I don't know whether that would be universally true.

This is a pretty extreme case, and currently there's no problem as there's only a TODO for logging, but I wanted to raise it early.

@WeiranFang
Copy link
Member

Hi @jskeet , thanks for the feedback! Just to make sure we are on the same page, so do you mean, in the case of PartitionRead or PartitionQuery method, even it uses the same session name to make the request, it can actually use different channels? If that's the case, does it mean that our current implementation (which forces those method to use the same channel because of the same session name) will cause a performance issue?

@jskeet
Copy link
Member Author

jskeet commented Sep 5, 2018

No, it's not PartitionRead or PartitionQuery that have the issue - it's ExecuteSql. Basically the process is:

  • Call PartitionQuery to get a load of partition IDs
  • Distribute those partition IDs and the session ID to multiple machines
  • On each of those multiple machines, call ExecuteSql passing in the original session ID and one of the partition IDs

It's the last step that's a potential problem. The different machines won't have a channel for that session, because the session was started on an entirely different machine - which means that if the TODO is ever fixed, Grpc.Gcp would log an error, despite nothing being wrong.

@WeiranFang
Copy link
Member

You are right. Thanks for pointing that out! I'll keep this issue open. When working on this TODO, we can either check for the exist of partition id (but we need to figure out all other cases with similar conditions), or lower the log level with detailed log messages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants