-
Notifications
You must be signed in to change notification settings - Fork 154
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
Refactor Database Model and Schema #253
Merged
Merged
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
MonitorCookies are marshalled as a json object, not an array. Fix it so that benchmark test works Signed-off-by: Adrian Moreno <[email protected]>
Signed-off-by: Adrian Moreno <[email protected]>
Clearly DBModel does not hold the full database model. Instead, only the combination of model.DBModel, mapper.Mapper and ovsdb.Schema is a useful database model as is used internally by libovsdb. The fact that server.go had to defined a struct called DatabaseModel with model.DBModel and ovsdb.Schema and dymanically create Mapper objects from them is a proof of this. In order to prepare for a DBModel refactoring, rename it to ClientDBModel as it's the client's view of the DatabaseModel. This patch does not contain functional changes Signed-off-by: Adrian Moreno <[email protected]>
Replace the one that server.go had to define. For now, it's just a drop-in replacement of the previous type. Signed-off-by: Adrian Moreno <[email protected]>
It is common to first create a DatabaseModel only based on the ClientDBModel, and then add / remove the schema to it when, e.g: when the client (re) connects. Validation is performed when the schema is added. Since the schema and mapper can be dynamically added and purged, protect them with a RWMutex Signed-off-by: Adrian Moreno <[email protected]>
@amorenoz need to fix lint errors 😉 |
dave-tucker
reviewed
Oct 25, 2021
dave-tucker
reviewed
Oct 25, 2021
dave-tucker
approved these changes
Oct 25, 2021
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of nits but overall this looks good to me. Great work @amorenoz
For the cache, it's simply replacing three fields with one For the client, use the 2-step DatabaseModel initialization Signed-off-by: Adrian Moreno <[email protected]>
Now that client, cache and server uses the DatabaseModel as central point of model creation and introspection, we can hide the dbModelClient and move its public functions to the DatabaseModel Signed-off-by: Adrian Moreno <[email protected]>
All around the codebase we're creating mapper.Info structures and then calling Mapper functions that create Info structures again. Given that mapper.Info already defines all the metadata that Mapper needs to do the native-to-ovs transations, it makes sense to use Info structures as input to all functions. That simplifies the code inside the mapper module. Also, I'd expect some performance improvement since we were creating multiple Info structs unnecessarily in the host path. It's true that, for now, it makes it sligthly more cumbersone to call mapper functions, since the Info struct has to be created first and it now requires an additional argument (the table name). However, this can be improved later on by having the database model build the Info structs for us. Signed-off-by: Adrian Moreno <[email protected]>
The core mapper API uses mapper.Info sctructs which can be created just by inspecting a TableSchema. However, having the DatabaseModel now centralizing accesses to the mapper API and containing both the Model types and the Schema, we can pre-create the mapper.Info.Metadata sctructs and cache them so we create Info sctructs more efficiently Signed-off-by: Adrian Moreno <[email protected]>
Pull Request Test Coverage Report for Build 1380776992
💛 - Coveralls |
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.
Right now, the way we use the
dbModel
and theSchema
is unclean and error prone.First, we have to hold pointers to them all around the code. Also, we use the
dbModel
to buildModel
instances but use theschema
to populate their fields which can lead to errors. As an example, if you take the current main branch, build the ovsdb-server and try to use cmd/stress on it, the client will crash with:In this case, although validation passes (having partial
Model
s is OK), the cache does not have thedbModel
handy when it's filling up the fields.Therefore, IMHO, it's clear that dbModel and schema are two sides of the same coin. The fact that server.go had to define a struct that holds both and is called DatabaseModel is a good proof of this:
libovsdb/server/server.go
Lines 31 to 34 in f2b3ce2
In addition,
model.Info
structures are created all over the code (e.g: api) that then callsMapper
functions that createmodel.Info
structures all over again.Try to improve all this by performing the following refactorings:
Rename current DbModel to ClientDBModel and introduce an internal DatabaseModel type
After the rename, a
DatabaseModel
type is introduced that combines both central objects that are core for the library and that supports 2-step initialization to allow the schema to be provided afterwards (on [re-]Connect()
). Validation is performed on the second step (without modifying the validation logic)mapper.Mapper
to usemapper.Info
Unify the
Mapper
into a more compact API by havingmapper.Mapper
functions accept amapper.Info
reference.Use the DatabaseModel to optimize the
mapper.Info
creationNow that we have unified all the model handling inside a single struct (
DatabaseModel
), use it to createmapper.Info
types. Since themapper.Info
is nothing but a metadata object attached to aModel
pointer, this allows for an optimization: when we validate the schema and theClientDBModel
we can create all the metadata objects and cache them. Creating a newmodel.Info
object is now faster as the Metadata object has already been created and stored .Reviewer notes: Although it provides little functionality change, this PR has a lot of modifications. I've tried to split the PR in commits that are easy to review