Unmerged commits from golang/protobuf #191
Comments
awalterschulze
changed the title
|
I've opened #319 to track merging up to golang/protobuf@748d386 - feel free to close that issue. |
|
Sorry for the delay. |
|
No worries, thanks! Could you roll forward up to golang/protobuf@1909bc2? |
|
done |
|
Sorry for the noob question, but what's the actual process for doing the update? I'm interested in locally updating gogoproto on top of upstream's dev branch to test out some recent performance improvements (golang/protobuf@8cc9e46), but the process for merging in new upstream commits doesn't appear to be documented. |
|
My guess is it's something like `git -C ../../golang/protobuf show 8cc9e46
| git apply`.
…On Thu, Nov 9, 2017 at 12:33 PM, Alex Robinson ***@***.***> wrote:
Sorry for the noob question, but what's the actual process for doing the
update? I'm interested in locally updating gogoproto on top of grpc's dev
branch to test out some recent performance improvements (golang/protobuf@
8cc9e46
<golang/protobuf@8cc9e46>),
but the process for merging in new upstream commits doesn't appear to be
documented.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#191 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPOiuyyztbTg9HzmXLOF_JBPmvGsHks5s0zdogaJpZM4JVEmk>
.
|
|
The process is: I manually do it when I get time using DiffMerge.app |
|
I have to read especially carefully in generator.go and jsonpb.go, since there is quite a big diff already. |
|
Sorry for the bad news. The process is really old school and painful. |
|
The merge process does look like a bit too much effort for me to do just for some early performance testing, so I'll just wait until it gets moved into the master branch there and eventually ends up here. Thanks for the quick reply and for all the work you do maintaining gogoproto! |
|
I agree :) Looks like its going to be a while before it is moved to master. So I should probably get started on my own dev branch. I don't know when I'll get time, but I hope it will be before they merge dev into their master. |
|
I have finally started work to merge the golang/protobuf Status of UpstreamOne package test is failing Todomerge protoc-gen-go/generator/generator.go merge proto/discard_test.go requires merge changes to generator.go merge proto/proto3_test.go requires Proto3UnknownFields update .proto files including well known types Done_conformance/conformance.go |
|
The golang/protobuf@8cc9e46 commit has now fully been merged into the gogoprotobuf upstream branch. I want to merge It would be nice to hear from at least one gogoprotobuf user that the upstream branch is working though, since this is such a large change. And I had to rewrite quite a few things from scratch. |
|
@awalterschulze yes, I'll test out the gogoprotobuf dev branch in cockroach this week. |
|
@a-robinson that would be awesome. Thank you so much. |
|
I'm not sure yet whether it's due to the upstream protobuf changes or to your tweaks in this repo, but I've hit a couple problems integrating the upstream branch into cockroach so far.
|
|
The smaller reproduction of running the test in https://github.com/cockroachdb/cockroach/blob/master/pkg/util/protoutil/clone_test.go also breaks: |
|
The relevant code for not allowing merges of non-pointer fields does appear to be new in the upstream branch: https://github.com/gogo/protobuf/blob/upstream/proto/table_merge.go#L534 |
|
Minimal repro: |
2.1 Could you provide a test that breaks merge, purely using the gogoprotobuf repo. Something we can commit as a regression test? 2.2 If you want to upstream your Clone function, that would also help to fix this long standing issue Does that cover everything, or did I miss something? Thank you so much for testing. This is really helpful. |
|
Ok, I think the Clone crash was actually just user error on my side. I can't get it to reproduce after coming back and rebuilding everything. I'm able to start a cockroach cluster and run our tests without issues. We can't switch to it particularly soon since we're in the midst of polishing off a major release right now, but should be ready to switch within 1-1.5 months if you think it'll be fully integrated by then. |
|
Ok wait. I just want to make sure. Are you saying everything works on the No worries about actual adoption. If it works for you, then I will merge |
I thought so, but apparently not. I had a non-upstream version of protoc-gen-gogo in my path that I think got used when I did my test this morning. There's something weird going on with golang's interpretation of types across packages that causes the But since it's now clear that the problem is in XXX_Merge, here's an easy repro that should work with your non-cockroach proto of choice that has a non-pointer struct field: |
|
Damn, I thought it was too good to be true. I don't think we are supposed to call XXX_Merge directly? And can you also provide non cockroach proto messages that we can use in a test and commit to gogoprotobuf, without adding any extra dependencies. |
|
It breaks with just: and |
|
And I don't know whether we're supposed to call XXX_Merge directly, but the code I linked to in |
|
I hope to finally work on this again this weekend. Is there a way you could
give me an example that does not call xxx methods directly. For example,
what happens when you just call proto.Merge?
…On Tue, 6 Mar 2018, 20:22 Alex Robinson, ***@***.***> wrote:
And I don't know whether we're supposed to call XXX_Merge directly, but
the code I linked to in proto.Merge (which gets called by proto.Clone)
does so.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#191 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABvsLdqFh4QUqXS3IUSV5xY4s6iynrI4ks5tbuIBgaJpZM4JVEmk>
.
|
|
I could reproduce it simply using proto.Clone or proto.Merge, without resorting to calling XXX methods directly. |
|
@a-robinson I have fixed the one particular case that you reported |
|
I have merged upstream into dev and I am now starting to merge the following commits on golang/protobuf's branch into gogo/protobuf's dev branch |
|
Testing of the up to date |
|
Sorry for the delay, I haven't had much spare time lately. I was able to successfully build cockroach and run a multi-node cluster, but ran into two problems when running tests.
|
|
1. What happens with xxx_unrecognized and yaml? Or what do you do to avoid
it? If you are not generating xxx_unrecognized then maybe we can add an
option to not generate these xxx fields either?
2. Am I right in guessing that this is because gogoprotobuf is not
supporting merge or clone? If so I am open to someone adding support. This
would require adding lots of tests. I will help to review. But I have never
had the time myself to add this support.
…On Mon, 9 Apr 2018, 04:17 Alex Robinson, ***@***.***> wrote:
Sorry for the delay, I haven't had much spare time lately.
I was able to successfully build cockroach and run a multi-node cluster,
but ran into two problems when running tests.
1. The internal XXX_NoUnkeyedLiteral and XXX_sizecache fields are
showing up in the output of yaml.Marshal on a proto, which seems
pretty undesirable:
--- FAIL: TestZoneConfigMarshalYAML (0.00s)
--- FAIL: TestZoneConfigMarshalYAML/#00 (0.00s)
zone_test.go:582: yaml.Marshal({RangeMinBytes:1 RangeMaxBytes:1 GC:{TTLSeconds:1 XXX_NoUnkeyedLiteral:{} XXX_sizecache:0} NumReplicas:1 Constraints:[] LeasePreferences:[] Subzones:[] SubzoneSpans:[] XXX_NoUnkeyedLiteral:{} XXX_sizecache:0})
got:
range_min_bytes: 1
range_max_bytes: 1
gc:
ttlseconds: 1
xxx_nounkeyedliteral: {}
xxx_sizecache: 0
num_replicas: 1
constraints: []
want:
range_min_bytes: 1
range_max_bytes: 1
gc:
ttlseconds: 1
num_replicas: 1
constraints: []
1. I hit a segfault inside the gogoproto code in a couple different
tests (TestBackfillErrors and TestLogic, on the same basic code path
each time:
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x1 pc=0x80adc0]
goroutine 130435 [running]:github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).computeMergeInfo.func1(0xc420b55558, 0xc422cde088)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:234 +0x8cgithub.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).merge(0xc423bc9e40, 0xc420b55548, 0xc422cde078)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:139 +0x110github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).computeMergeInfo.func27(0xc420b55548, 0xc422cde078)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:536 +0x3egithub.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).merge(0xc423bc9b40, 0xc420b55530, 0xc422cde060)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:139 +0x110github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).computeMergeInfo.func27(0xc420b55530, 0xc422cde060)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:536 +0x3egithub.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).merge(0xc423bc9340, 0xc420b55500, 0xc422cde030)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:139 +0x110github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*InternalMessageInfo).Merge(0x368c160, 0x261db40, 0xc420b55500, 0x261db40, 0xc422cde030)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:50 +0x58github.com/cockroachdb/cockroach/pkg/sql/sqlbase.(*TableDescriptor).XXX_Merge(0xc420b55500, 0x261db40, 0xc422cde030)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/sqlbase/structured.pb.go:1328 +0x57github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.Merge(0x261db40, 0xc420b55500, 0x261db40, 0xc422cde030)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/clone.go:95 +0x277github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.Clone(0x261db40, 0xc422cde030, 0xc422cde030, 0x261db40)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/clone.go:52 +0x168github.com/cockroachdb/cockroach/pkg/util/protoutil.Clone(0x2634a80, 0xc422cde030, 0x2601500, 0xc420454db0)
/go/src/github.com/cockroachdb/cockroach/pkg/util/protoutil/clone.go:69 +0x92github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.ConvertBackfillError(0x2626c80, 0xc422957d80, 0xc422cde030, 0xc421b0ee00, 0x2601500, 0xc420454db0)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/backfiller.go:310 +0x4cgithub.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*indexBackfiller).runChunk.func2.1(0x2626c80, 0xc422957d80, 0xc42567cb60, 0x1, 0xc421f06e38)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/indexbackfiller.go:211 +0x26cgithub.com/cockroachdb/cockroach/pkg/internal/client.(*DB).Txn.func1(0x2626c80, 0xc422957d80, 0xc42567cb60, 0xc421f06e38, 0x1523a2624a3e3557, 0x0)
/go/src/github.com/cockroachdb/cockroach/pkg/internal/client/db.go:516 +0x43github.com/cockroachdb/cockroach/pkg/internal/client.(*Txn).Exec(0xc42567cb60, 0x2626c80, 0xc422957d80, 0xc425670101, 0xc42052c480, 0xc424e13870, 0xc424e13720)
/go/src/github.com/cockroachdb/cockroach/pkg/internal/client/txn.go:753 +0xfegithub.com/cockroachdb/cockroach/pkg/internal/client.(*DB).Txn(0xc420b12390, 0x2626c80, 0xc422957d80, 0xc423782180, 0x0, 0x0)
/go/src/github.com/cockroachdb/cockroach/pkg/internal/client/db.go:515 +0x133github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*indexBackfiller).runChunk.func2(0x2626d40, 0xc423cd9620, 0xc400000002, 0x2311c7e)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/indexbackfiller.go:200 +0xa6github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*indexBackfiller).runChunk(0xc422cde000, 0x2626d40, 0xc423cd9620, 0xc423cd9650, 0x1, 0x1, 0xc42385cc88, 0x6, 0x38, 0xc42003fc48, ...)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/indexbackfiller.go:276 +0x745github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*backfiller).mainLoop(0xc422cde000, 0x2626d40, 0xc423cd9620, 0x22c645c, 0x8)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/backfiller.go:137 +0x64agithub.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*backfiller).Run(0xc422cde000, 0xc42159aac0)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/backfiller.go:89 +0x1fa
created by github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*Flow).Start
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/flow.go:481 +0x393
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x1 pc=0x80adc0]
goroutine 22949 [running]:github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).computeMergeInfo.func1(0xc4215b2e58, 0xc425c7d288)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:234 +0x8cgithub.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).merge(0xc427a89c80, 0xc4215b2e48, 0xc425c7d278)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:139 +0x110github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).computeMergeInfo.func27(0xc4215b2e48, 0xc425c7d278)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:536 +0x3egithub.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).merge(0xc427a89940, 0xc4215b2e30, 0xc425c7d260)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:139 +0x110github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).computeMergeInfo.func27(0xc4215b2e30, 0xc425c7d260)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:536 +0x3egithub.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*mergeInfo).merge(0xc427a889c0, 0xc4215b2e00, 0xc425c7d230)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:139 +0x110github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.(*InternalMessageInfo).Merge(0x340bfa0, 0x2434580, 0xc4215b2e00, 0x2434580, 0xc425c7d230)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/table_merge.go:50 +0x58github.com/cockroachdb/cockroach/pkg/sql/sqlbase.(*TableDescriptor).XXX_Merge(0xc4215b2e00, 0x2434580, 0xc425c7d230)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/sqlbase/structured.pb.go:1328 +0x57github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.Merge(0x2434580, 0xc4215b2e00, 0x2434580, 0xc425c7d230)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/clone.go:95 +0x277github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto.Clone(0x2434580, 0xc425c7d230, 0xc425c7d230, 0x2434580)
/go/src/github.com/cockroachdb/cockroach/vendor/github.com/gogo/protobuf/proto/clone.go:52 +0x168github.com/cockroachdb/cockroach/pkg/util/protoutil.Clone(0x2448140, 0xc425c7d230, 0x2419800, 0xc4291d9800)
/go/src/github.com/cockroachdb/cockroach/pkg/util/protoutil/clone.go:69 +0x92github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.ConvertBackfillError(0x243b380, 0xc427a88540, 0xc425c7d230, 0xc42d963100, 0x2419800, 0xc4291d9800)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/backfiller.go:310 +0x4cgithub.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*indexBackfiller).runChunk.func2.1(0x243b380, 0xc427a88540, 0xc42f9c8b60, 0x1, 0xc4320849d0)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/indexbackfiller.go:211 +0x26cgithub.com/cockroachdb/cockroach/pkg/internal/client.(*DB).Txn.func1(0x243b380, 0xc427a88540, 0xc42f9c8b60, 0xc4320849d0, 0x1523a258ab5bea0a, 0x0)
/go/src/github.com/cockroachdb/cockroach/pkg/internal/client/db.go:516 +0x43github.com/cockroachdb/cockroach/pkg/internal/client.(*Txn).Exec(0xc42f9c8b60, 0x243b380, 0xc427a88540, 0xc42f9c0101, 0xc4291d9780, 0xc432775870, 0xc432775720)
/go/src/github.com/cockroachdb/cockroach/pkg/internal/client/txn.go:753 +0xfegithub.com/cockroachdb/cockroach/pkg/internal/client.(*DB).Txn(0xc420fbe390, 0x243b380, 0xc427a88540, 0xc431a74fe0, 0x0, 0x0)
/go/src/github.com/cockroachdb/cockroach/pkg/internal/client/db.go:515 +0x133github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*indexBackfiller).runChunk.func2(0x243b440, 0xc42a8d38f0, 0xc400000002, 0x2170414)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/indexbackfiller.go:200 +0xa6github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*indexBackfiller).runChunk(0xc425c7d200, 0x243b440, 0xc42a8d38f0, 0xc42a8d3920, 0x1, 0x1, 0xc4320843c8, 0x2, 0x8, 0xc4320843d0, ...)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/indexbackfiller.go:276 +0x745github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*backfiller).mainLoop(0xc425c7d200, 0x243b440, 0xc42a8d38f0, 0x212ecaf, 0x8)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/backfiller.go:137 +0x64agithub.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*backfiller).Run(0xc425c7d200, 0xc42d269b40)
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/backfiller.go:89 +0x1fa
created by github.com/cockroachdb/cockroach/pkg/sql/distsqlrun.(*Flow).Start
/go/src/github.com/cockroachdb/cockroach/pkg/sql/distsqlrun/flow.go:481 +0x393
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#191 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABvsLYnkXkBFWZwb5n8UHhyxScVjJJw9ks5tmsSlgaJpZM4JVEmk>
.
|
|
Hi, I know this is an old issue but I'm working on updating dependencies for cockroach and hit this issue.
I think the generated code for those fields should have |
|
@awalterschulze can you expand on gogoprotobuf not supporting merge/clone? There is plenty of code for this, e.g. in https://github.com/gogo/protobuf/blob/master/proto/table_merge.go. Is this code known not to work? |
|
This patch makes things work for us. Would you be open to making a change like this, even if we don't promise full support for Clone? |
|
There are three (two new) extensions that you can use to turn off the generation of XXX fields so you don't have any problems with yaml: |
|
Merge and Clone are not supported, because someone has not taken the time to write tests for a lot of combinations and made sure that it is working. Or in other words, I was lazy :) I believe @jmarais and @donaldgraham the new maintainers of gogoprotobuf would be open to this support being added, but you would have to at least provide test cases, to make sure that no regression occurs. That way limited support could be grown into full support over time. I hope that helps Side note: As a rule: unsafe imports should only be added to files that have the unsafe build tag. |
|
Thanks! I will work on a PR for the clone thing. On the yaml thing - those two extensions are not in the last release unfortunately (and the XXX fields are). Is there a catch with using By the way, do you have a sense of when the next release might be? |
|
The catch is you have to use the gogo/protobuf/proto library or the
generated methods to marshal and unmarshal, which is the usual catch with
using gogoprotobuf extensions :)
Good job on finding a work around.
@donaldgraham and @jmarais would have to answer about the release.
…On Sun, 9 Dec 2018 at 02:59, RaduBerinde ***@***.***> wrote:
Thanks! I will work on a PR for the clone thing. On the yaml thing - those
two extensions are not in the last release unfortunately (and the XXX
fields are). Is there a catch with using goproto_sizecache? (I assume the
field is used for something?) Anyway, I worked around it by implementing a
custom MarshalYAML.
By the way, do you have a sense of when the next release might be?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#191 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABvsLc7v_WAkJiKN5DmBy3v7uFU9abqeks5u3HyNgaJpZM4JVEmk>
.
|
gogoprotobuf: All commits on master up to Aug 27, 2019 have been merged - golang/protobuf@822fe56
This message is repeatedly edited to reflect the newest merged commit
The text was updated successfully, but these errors were encountered: