Skip to content

Conversation

@songgao
Copy link

@songgao songgao commented Dec 3, 2016

r? @maxtaco @patrickxb Thanks!

This should make TLFMessage (as opposed to TlfMessage) possible

@maxtaco
Copy link
Contributor

maxtaco commented Dec 3, 2016

what will this break? we can't be breaking existing KBFS clients. Will this break those?

@songgao
Copy link
Author

songgao commented Dec 3, 2016

Not from where I see. When we update protocols in client, it breaks Go code, which should be easy to fix from compiler errors. Without updated vendor in KBFS, it would remain the same, and since codec doesn't change, it wouldn't change message on-the-wire. Then we can update vendor in KBFS. It seems to me that nothing will break; we'll just be updating names in Go code. Does that sound correct?

@maxtaco
Copy link
Contributor

maxtaco commented Dec 3, 2016

In general, we further require that all local keybase processes work with other keybase processes from earlier versions, since restarts don't happen all at once. Seems like this change will break backwards/forwards compatibility between kbfs and the keybase service.

@maxtaco
Copy link
Contributor

maxtaco commented Dec 3, 2016

I don't think this change is worth the risk. The risk is that our software will break in weird ways in the wild. The upside is extremely limited. Users won't benefit at all. Our team will see it as slightly more consistent.

@songgao
Copy link
Author

songgao commented Dec 3, 2016

Between KBFS and service it's RPC, which is done using the so-called over-the-wire encoding right? The binary is always one file and when updated, it's the entire process

@maxtaco
Copy link
Contributor

maxtaco commented Dec 3, 2016

As I said above, we've had the requirement that even local upgrades don't break backwards compatibility. We've definitely had issues in the past with upgrades that did.

@songgao
Copy link
Author

songgao commented Dec 3, 2016

I still don't under how this breaks backward compatibility. It doesn't change message on-the-wire, thus RPCs should still work like before ...

@maxtaco
Copy link
Contributor

maxtaco commented Dec 3, 2016 via email

@maxtaco
Copy link
Contributor

maxtaco commented Dec 3, 2016

Ok thanks for settting me straight. We can do this but it is a big change on the go side. Is it worth the headache? The status quo doesn't bother me much. But I agree it won't break wire compatibility.

@songgao
Copy link
Author

songgao commented Dec 3, 2016

No problem; Thanks Max!

There are renaming and refactoring tools from gorename and gofmt that makes this easier. I think if we can get this done in a fairly short time, it won't bother many people. Also since we are doing vendoring, I think this can be done repo-by-repo without breaking CI.

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.

3 participants