Protobuf: guidelines and best practices for deprecating fields

[Allen Webb]

I would like to discuss guidelines for how we handle deprecated proto fields in Chrome OS. My recommendation is to prefer replacing the field with a reserved statement unless code is needed for migrating old protos with the deprecated field in which case the "deprecated" field option can be used. We clearly need some placeholder to avoid the tag for the old field being reused in an incompatible way. However, I don't believe it makes sense to rename the field now that the deprecated option is available.

As part of the work to upgrade protobuf on Chrome OS from 3.7.0 to 3.11.4 I ran into one breaking change: The protobuf library now generates warnings when deprecated code is used. This is nice for helping refactor code that uses deprecated APIs, but it also breaks code that uses proto fields with the [deprecated = true] option (even code generated by protoc). I have CLs to fix existing code, but it would be helpful to have some agreement on how things should be handled going forward.

Any objections to making reserved fields the preferred choice for deprecated fields with a fallback to [deprecated = true] and -Wno-error=deprecated-declarations for migration code?

Also, is anyone aware of interesting edge cases to consider?

[Mike Frysinger]

this would be my prefered guideline.  it's the approach we've been using in chromite's BuildAPI at least.

Also, is anyone aware of interesting edge cases to consider?

glancing through the system_api tree, i see:

• removal of the field and replacing it with the "reserved" keyword
• use of the [deprecated = true] tag Allen points out
• renaming the field to include OBSOLTE_ or similar name mangling
• a comment in the proto file saying the field is deprecated, but no other process to actually enforce/clean it up

but it's not clear we couldn't use the delete+reserved keyword for all of these situations

-mike

[Kevin Chowski]

Take my opinion with a grain of salt: I have no experience with protos in Chrome OS, but plenty of experience in other orgs.

In an ideal world one would be able to delete+reserve fields the moment they are no longer needed, and often in self-contained situations that is possible. I would agree this is the preferred option if it is possible - no reason to have fields sitting around that aren't used or useful. But sometimes that's not feasible to do that, particularly when you don't have tight and absolute control over updates, so I think the suggested fallback of setting deprecated=true would work better than not marking it deprecated in most cases.

For an example of when it's infeasible to delete+reserve immediately, say there is a message type X which has a field 'a' that is being set already in some code (the "writer") that already is launched and already is recording messages of type X regularly; let's also assume that somewhere else I have another program (the "reader") which is reading this 'a' field and doing some stuff with it, and then discards the rest of the X message. If I want to change the "writer" code to instead write to a new field 'b', I can't just delete+reserve the field 'a' at the same time: even if I could easily remove all references to 'a' in the "writer" code in one fell swoop, I still want to be able to read all of the data any existing "writer"s are writing until all of those binaries are updated with the newer version which writes field 'b'. This necessitates leaving both fields in the proto at the same time, but (assuming the intent is to fully migrate from field 'a' to 'b') it seems like the 'a' field could be marked deprecated to help avoid people adding new dependencies on it. Unfortunately, depending on the release cycle of the "writer", the field may have to be deprecated for many months or perhaps indefinitely. I don't love the idea of having a deprecated field that is intentionally going to be left there for a long time, but it seems better than any of the alternatives...

Even for slightly more controlled situations where you do fully control the update cycle for readers/writers of some proto, it can still be tricky to do a total migration all at once. As another example, storing proto messages persistently can cause a similar problem as before. If you want to move from Y.j to Y.k, but you're storing messages of type Y on/in a disk/database, then I see three options: 1) accept you've lost all historical data in field 'j'; 2) write some wrapping logic to encapsulate the Y message, allowing for code  to use a single API to access either 'j' or 'k' automatically; 3) write a backfill program which mechanically changes 'j' to 'k' in the persistent storage. Option #1 allows you to delete+reserve immediately; for option #2 you have to leave the field in the message indefinitely; option #3 is similar to the above example, where you can eventually delete+reserve the field but both have to exist in the proto message at some point in order for the backfill code to be checked in; in this case setting the old field to deprecated makes sense, assuming the backfill is going to happen ~soon. Option #2 may sound like a bad idea on first consideration, but presuming sufficient encapsulation in the wrapper type I've seen this work out fine in some large projects. It really depends on the circumstance and how complicated the wrapper type needs to be. But this is the case it's not clear to me whether marking the old field as deprecated is right: it somewhat depends on the situation, e.g. who is setting fields of that proto, a client on its own or is it being set via the wrapper type.

--
--
Chromium OS Developers mailing list: chromiu...@chromium.org
View archives, change email options, or unsubscribe:
https://groups.google.com/a/chromium.org/group/chromium-os-dev
---
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-os-d...@chromium.org.

[Ned]

+Andrew Lamb +Sean Abraham who has been thinking a lot about these topics as well