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

Write impl/prod directly in terms of the protobufs? #23

Open
riannucci opened this issue Nov 25, 2015 · 5 comments
Open

Write impl/prod directly in terms of the protobufs? #23

riannucci opened this issue Nov 25, 2015 · 5 comments

Comments

@riannucci
Copy link
Member

Not sure how hard this would be, but it might be easier than it seems. This would allow the removal of all of the reflection code in the 'Raw' paths for impl/prod (because currently the SDK does some reflection, but we try to bypass/shortcut it).

This might be a long-term maintenance issue, however, if the protos change frequently/semi-frequently.

@riannucci
Copy link
Member Author

Not only would this allow removal of the extra reflection code in impl/prod, but it would enable:

  • cleaner transaction semantics (e.g. explicit TransactionID in context, which inspectable, closable, settable, nillable)
  • cleaner namespace semantics (could actually remove a namespace, which is currently impossible with the actual SDK)
  • could actually implement Make impl/memory compatible with appengine.APICallFunc hooks to allow existing codebases to use it. #37
  • could potentially even turn luci/gae into the basis for a (fast) dev_appserver replacement (which would bring speedup benefits to other appengine languages). Could potentially design this one to support multiple connections with separate datastore instances, so that unittests in other languages could each operate with a clean datastore. Could potentially add RPCs to control e.g. the Testable() interface methods on a per-connection basis.

Getting really frustrated around transaction * namespace manipulation semantics.

@riannucci
Copy link
Member Author

Oh, and it would also mean that some of the custom encodings could be replaced with the actual protos (e.g. Property objects could have more-native binary representations), which might further reduce the amount of code in e.g. impl/memory.

@riannucci
Copy link
Member Author

If the transaction and namespace are explicit, this would also allow us to remove the filter apis in favor of simply extracting the current client, applying a filter to it, and putting the new, filtered, client back into the context, which I think @vadimsht would like (really: everyone would like that, but I know vadim's been bugged by the filter API in the past, and I'm personally not a huge fan of it either) 😄

@riannucci
Copy link
Member Author

Ran into the transaction crap again when implementing DM: there's no way in a tumble mutation to ditch the current transaction when handing the context off to a distributor implementation. For now distributor implementations will have to use the GetNoTxn methods, but it would be much better if we could just do a c = gae.SetTransaction(c, nil).

@riannucci
Copy link
Member Author

And GetNoTxn is still terrible: If the method which needs to ignore the transaction passes the context to some other method, that inner method also needs to be written to ignore the current transaction. Ugh.

chromium-infra-bot pushed a commit that referenced this issue Jun 2, 2016
I really don't like this but short of fixing #23, I don't think there's much we
can do.

[email protected], [email protected]
BUG=

Review-Url: https://codereview.chromium.org/2036593002
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

1 participant