Skip to content

Conversation

@gautamomento
Copy link
Contributor

Given GRPC OK Responses aren't logged (rightfully so). There are still bugs that can be encountered if the response content is incorrect.

e.g. SetResponse should only have ECacheResult.OK and GetResponse should only support ECacheResult.Hit or ECacheResult.Miss.

Any other ECacheResult is indicative of a bug in MomentoSdk or on SCS. Throwing an exception with generic message will hide the underlying issue. Hence, setting an explicitmessage for such exceptions so any reports can be easily worked on immediately instead of having to tell customers to enable debug logging in the SDK. Enabling debug logging on customer's environments will take time and also same logging related challeneges would apply.

Given GRPC OK Responses aren't logged (rightfully so). There are still bugs that can be encountered if the response content is incorrect.

e.g. SetResponse should only have ECacheResult.OK and GetResponse should only support ECacheResult.Hit or ECacheResult.Miss.

Any other ECacheResult is indicative of a bug in MomentoSdk or on SCS. Throwing an exception with generic message will hide the underlying issue. Hence, setting an explicitmessage for such exceptions so any reports can be easily worked on immediately instead of having to tell customers to enable debug logging in the SDK. Enabling debug logging on customer's environments will take time and also same logging related challeneges would apply.
Copy link

@danielamiao danielamiao left a comment

Choose a reason for hiding this comment

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

I'm fine with this change as-is, but I'm still unsure what happens if we decide to add additional states (for whatever reason) in the future. Does it mean as soon as MR2 updates to new protos, old SDK clients will throw this exception? I forget how versioning across new protobufs work...

@gautamomento
Copy link
Contributor Author

I'm fine with this change as-is, but I'm still unsure what happens if we decide to add additional states (for whatever reason) in the future. Does it mean as soon as MR2 updates to new protos, old SDK clients will throw this exception? I forget how versioning across new protobufs work...

Yes you are right that this will cause problems if and when we introduce new states valid states for these operations. The more I think about it, probably I should stop requiring certain statuses. Probably it should map as

Hit -> Sdk.Hit
Miss -> Sdk.Miss
Anything else -> Sdk.Unknwon

However, GetResponse also has data. The way we expose this is that we only return data if the Status is Hit. What contract should we establish for Unknwon status, there may or may not be data.

I will give this a thought.

@danielamiao
Copy link

@gautamomento yeah let me know if you want to get together to discuss this as a group once you give it more thought, I'm curious to get @kvcache's thoughts as well.

@kvcache
Copy link
Contributor

kvcache commented Jan 6, 2022

What contract should we establish for Unknwon status, there may or may not be data.

I think if the status is unknown, the data's interpretation is unknown. According to the mental model of the world at this time, there is only HIT or MISS for get(), and there is only OK for set(). Anything else we do not have handling for.

I would personally recommend either:

  • raising an exception
  • returning an object with the unknown code and the raw proto as fields; and make sure there's a nice __str__(self) and __repr__(self) that our users can conveniently read and report to us in their own logs. (expecting they'll probably be hip to doing a log.error(unknown_result) before even emailing us)

@gautamomento
Copy link
Contributor Author

  • raising an exception

The argument against raising an exception is future proofing for any new ECacheResult statuses that may be valid responses.

e.g. If we end up adding ECacheResult.AlreadyExists when someone tries to Set a key that is present would cause older SDKs to 😢 since they always expect Ok.

However, I am largely convinced that Get(Hit, Miss) and Set(Ok) are the only high-level Results that we will expect. If we ever need any additional details - I think that can be best done by adding a new field that conveys the granular details.

enum ResultDetails {
 ALREADY_EXISTS,
}

message SetResponse {
  ECacheResult result = 1;
  string message = 2;
  ResultDetails result_details = 3;
}

I remain in favor of raising exceptions the way we are.

@gautamomento gautamomento merged commit 56817ab into main Jan 7, 2022
@gautamomento gautamomento deleted the u/gautamomento/make-invalid-ecacheresult-explicit branch January 7, 2022 00:01
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

Successfully merging this pull request may close these issues.

4 participants