catalog,sqlbase,*: generalize descriptors, pass around wrapper structs #49960
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ajwerner
requested review from
miretskiy,
rohany and
thoszhang
and removed request for
a team
|
This change is |
rohany
reviewed
Jun 8, 2020
ajwerner
force-pushed
the
ajwerner/wrapped-descriptors
branch
from
de1ec78
to
db3a0bf
Compare
rohany
reviewed
Jun 8, 2020
rohany
reviewed
Jun 8, 2020
|
Overall, I'm on board with the changes made in this PR |
ajwerner
force-pushed
the
ajwerner/wrapped-descriptors
branch
3 times, most recently
from
df92ae2
to
237d2d0
Compare
This commit modifies structured.proto to duplicate the fields required for leasing into each of the descriptor types. It does not yet adopt these fields. This commit is part of a much larger change to wrap descriptor proto structs used throughout the codebase. To contextualize the scope of this change, in an earlier iteration this commit, instead of duplicating these fields, opted to create a new message which was to be stored in the `Descriptor` type itself. While that change brought with it some niceties, it also brought the requirement to wrap the raw descriptor structs as they no longer carried all of their relevant metadata. This duplication approach in many ways obviates the pressing need for much of this PR but, nevertheless, we pursue it because it's part of a broader trend we'd like to enforce. In many ways the scope of this PR has become opportunistic rather than necessary. Release note: None
For now, move the Descriptor interface down into sqlbase as part of the effort to elimate sqlbase.DescriptorProto. Release note: None
In preparation for merging that interface with DescriptorInterface. Release note: None
This change is large but is largely mechanical. The basic idea is that we want to stop passing around raw pointers to protobuf structs. Instead we'd like to pass around higher-level wrappers which implement interfaces. There's some room for debate about the precise nature of Mutable/Immutable wrappers for descriptor types but over time, hopefully, we'll move descriptor manipulation behind clear interfaces. It's worth noting that this commit is a half-step. It began, somewhat unfortunately, by introducing the TypeDescriptor wrappers and then goes all the way to deleting the `DescriptorProto` interface and unexporting `WrapDescriptor`. Release note: None
…escriptor Next we'll stop having the raw descriptor implement that interface. Release note: None
This is a large but also mostly mechanical change. There's room for improvement on the uniformity of it all but let's leave that be for now as I'm getting tired and need to keep making progress. It'll be easier to clean it up later. In particular, I've increasingly been feeling that an interface-based approach to these descriptors would be worthwhile and that that would be easy to accomplish for database but alas. That will have to be another big and mechanical change. Release note: None
This commit has GetDescriptorByID return an "unwrapped" descriptor and removes `UnwrapDescriptor` function from sqlbase. This terminology is all crap given the `UnwrapDescriptor` function returned a descriptor that is itself sort of wrapped. Good news, this terminology is going away. There is a hodge-podge of other unfortunate touch up in this commit. Release note: None
Release note: None
This commit made a lot more sense when it removed those fields from the proto and lifted them to a new message. It's still not a terrible commit so we'll leave it. It should be handy when we transition descriptor interactions to be through interfaces. Release note: None
This commit, like others, is not nearly as urgent given we're keeping the meta fields on the database descriptor. This commit adopts the interface methods for accessing the ID and Name of a DatabaseDescriptor. The reworking in this commit of previous changes also in this PR is annoying, I'm sorry to the reviewers. I'm also increasingly sensing the urgency of eschewing the concrete descriptor wrappers in favor of interfaces but I'm not going to try to deal with that in this PR. Release note: None
In anticipation of lifting methods to the wrapper types. Release note: None
Release note: None
This commit makes more retrieval functions "unwrap" descriptors into DescriptorInterface before returning them. It somewhat unfortunately pushes that responsibility into the sql/resolver in order to accomodate backups. Release note: None
This commit is the ugly side of copying the leasing fields into each of the descriptors. This leaves the `Descriptor.Table` method in place for now as it forms defense in depth. We'll remove it eventually when this all gets cleaned up. Release note: None
ajwerner
force-pushed
the
ajwerner/wrapped-descriptors
branch
from
237d2d0
to
d5229de
Compare
thoszhang
approved these changes
Jun 9, 2020
|
TFTR! bors r=rohany,lucy-zhang |
Build succeeded |
|
Can't believe that made it in on the first bors try |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR works towards enabling unified leasing of descriptors. I'm somewhat dissatisfied at the state it ends up leaving things but it is forward progress and I can't keep holding on to this.
This PR began its life with an intention of creating a unified
DescriptorMetaproto message which would contain shared fields and live on theDescriptor. That change would necessitate not passing around the raw members ofDescriptor.Unionas they would no longer retain the necessary fields. Ultimately, after this was mostly done, it was determined that that change was not worth the migration pain. Nevertheless, a good bit of work was done to plumb through the new higher level abstractions and that work it valuable.The biggest issue with reviewing this change will be
sqlbase,*: replace DescriptorProto with DescriptorInterface, that commit does more than it should in a single commit but the time it is going to take to rip it apart doesn't seem obviously worth it given the overhead of keeping this change in sync.I want to re-iterate that this change represents a stepping stone and not an end-state. The plan is to proceed from here by ripping apart sqlbase into subpackages such that the implementations of each of the wrapper types lives in their own packages. Then the plan will be to replace the use of all concrete fields in the descriptor types with interfaces. At that point the
MutablevsImmutabledistinction can be rephrased as an interface distinction.Doing the above should also aid in a number of other ways.