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

Add support for stable Func keys in App Engine second gen #184

Merged
merged 11 commits into from Dec 17, 2018

Conversation

sbuss
Copy link
Contributor

@sbuss sbuss commented Dec 8, 2018

Fixes #122

@zombiezen
Copy link
Contributor

Question about requirements: does this need to preserve compatibility with a previous scheme for encoding function names? If not, then may I suggesting using the value of Frame.Function from runtime.CallersFrames (example)? Seems more stable than reverse engineering file paths, and has the added benefit of not requiring a new exported symbol.

@sbuss
Copy link
Contributor Author

sbuss commented Dec 10, 2018

does this need to preserve compatibility with a previous scheme for encoding function names?

Unfortunately, yes. The scheme is described here: #122 (comment)

@zombiezen
Copy link
Contributor

Gotcha, that was the context I was missing. I'll review with that in mind.

Copy link
Contributor

@zombiezen zombiezen left a comment

Choose a reason for hiding this comment

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

I think you're on the right track here now that I understand the issue more fully. Since we are codifying something that is kinda fragile to begin with, I want to make sure we proceed carefully.

appengine.go Outdated Show resolved Hide resolved
appengine.go Outdated Show resolved Hide resolved
delay/delay.go Outdated Show resolved Hide resolved
delay/delay.go Outdated Show resolved Hide resolved
delay/delay_test.go Show resolved Hide resolved
delay/delay.go Outdated Show resolved Hide resolved
delay/delay.go Show resolved Hide resolved
delay/delay.go Outdated Show resolved Hide resolved
@broady
Copy link
Contributor

broady commented Dec 10, 2018

Since we are codifying something that is kinda fragile to begin with

+1, and more generally, we should be very relaxed with any guarantee for this package. It's inherently flaky, and people should know that going in.

@sbuss
Copy link
Contributor Author

sbuss commented Dec 10, 2018

It's inherently flaky, and people should know that going in.

How should I document that? @broady

@broady
Copy link
Contributor

broady commented Dec 11, 2018

How should I document that? @broady

Package-level doc, maybe? I think it's in the docs on cloud.google.com somewhere, but fairly hidden.

delay/delay_test.go Show resolved Hide resolved
delay/delay.go Outdated Show resolved Hide resolved
internal/main_vm.go Show resolved Hide resolved
internal/main_vm.go Outdated Show resolved Hide resolved
internal/main_vm.go Outdated Show resolved Hide resolved
internal/main_vm.go Outdated Show resolved Hide resolved
Copy link
Contributor

@zombiezen zombiezen left a comment

Choose a reason for hiding this comment

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

LGTM, please wait for @broady's approval.

@sbuss
Copy link
Contributor Author

sbuss commented Dec 12, 2018

Package-level doc, maybe? I think it's in the docs on cloud.google.com somewhere, but fairly hidden.

@broady WDYT of ac8a57f

@sbuss sbuss merged commit e9657d8 into master Dec 17, 2018
@sbuss sbuss deleted the stable-func-second-gen branch December 17, 2018 20:59
@sbuss sbuss added this to the Go 1.11 milestone Feb 25, 2019
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.

None yet

3 participants