-
Notifications
You must be signed in to change notification settings - Fork 151
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: Reduce Use Of Pointers #256
Conversation
Pull Request Test Coverage Report for Build 1395949233
💛 - Coveralls |
Ok this snowballed quickly. Aside from the original code required, this also fixes:
|
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.
Can we measure the memory implications of this patch?
@@ -147,7 +147,7 @@ func (r *RowCache) Create(uuid string, m model.Model, checkIndexes bool) error { | |||
if err != nil { | |||
return err | |||
} | |||
newIndexes := newColumnToValue(r.dbModel.Schema().Table(r.name).Indexes) | |||
newIndexes := newColumnToValue(r.dbModel.Schema.Table(r.name).Indexes) |
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.
Schema.Table(r.name) can still return nil if the schema is cleared before the cache is updated, right?
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.
good point, i was so focussed on Schema()
not panicking I forgot that Table()
could too.
let me see if I can fix that one as well...
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.
In fact, I think I might defer that to a follow up to fix the cache without using pointers 🤔
comparing 🤯 |
@dave-tucker, I'm frankly confused |
@@ -778,16 +795,6 @@ func (o *ovsdbClient) monitor(ctx context.Context, cookie MonitorCookie, reconne | |||
} else { | |||
args = ovsdb.NewMonitorArgs(dbName, cookie, requests) | |||
} | |||
|
|||
// if we're reconnecting, we already hold the rpcMutex |
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.
why is this being removed in this commit?
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.
It get's moved to the top of the function here: https://github.com/ovn-org/libovsdb/pull/256/files#diff-bd3a55a72186f59e2e63efb4951573b2f9e4a7cc98086e922b0859f8ccc1dd09R759
This is to ensure that we fail early if we're disconnected 😉
db.schemaMutex.Unlock() | ||
db.modelMutex.Lock() | ||
var errors []error | ||
db.model, errors = model.NewDatabaseModel(schema, db.model.Client()) |
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.
is this some other fix unrelated to pointer conversion?
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.
No, this is related to the change from using a pointer to DatabaseModel
When DatabaseModel
was a pointer we'd:
- Create
NewPartialDatabaseModel
first - Call
model.SetSchema
once we've connected and got the schema
Now because the data is immutable we:
- Create
NewPartialDatabaseModel
first - Replace the stored model with the result of
NewDatabaseModel
once we've connected and got the schema
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.
i see, thanks
@@ -564,15 +564,16 @@ func (t *TableCache) Populate(tableUpdates ovsdb.TableUpdates) error { | |||
} | |||
tCache := t.cache[table] | |||
for uuid, row := range updates { | |||
t.logger.V(5).Info("processing update for row", "uuid", uuid, "table", table) | |||
logger := t.logger.WithValues("uuid", uuid, "table", table) |
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.
no concerns about performance here right? I just took a quick peek at the this function and it calls a read lock on the delegating logger:
https://github.com/kubernetes-sigs/controller-runtime/blob/v0.10.2/pkg/log/deleg.go#L184
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.
No I'm pretty sure this is fine, since it's a usage pattern that's documented: https://pkg.go.dev/github.com/go-logr/logr#hdr-Saved_Values
@@ -640,7 +640,7 @@ func (t *TableCache) Populate2(tableUpdates ovsdb.TableUpdates2) error { | |||
case row.Modify != nil: | |||
existing := tCache.Row(uuid) | |||
if existing == nil { | |||
return fmt.Errorf("row with uuid %s does not exist", uuid) | |||
return NewErrCacheInconsistent(fmt.Sprintf("row with uuid %s does not exist", uuid)) |
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.
should the goal here be eventually that populate and populate2 return typed errors so that we dont assume every error on populate /populate2 is an inconsistent cache problem and force reinit of the cache?
020a3bd#diff-74597f1e11b4f598d4f75e99aee5237cc9dc7e7cf008e1f33e3c72dc4a93ffe3R518
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.
Yeah it's better to return typed errors here because we have a better idea of what went wrong vs. wrapping them all later.
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.
besides the one remaining comment about the wip bugfix commit being squashed, lgtm
This commit stops using pointers for a number of structures. For example: ovsdb.DatabaseSchema, model.DatabaseModel Pointers are great for values that need to be mutable. But in all the cases changed, we don't need to mutate these values. Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
1. Don't add the `WithValues("model", dbName)` on every call to `Connect()` because that API can be called many times and the k/v pairs are added each time! 2. Don't register metrics in the `Connect()` method for the same reason 3. In `cache.go` create a new logger in each `populate` to carry the `uuid` and `table` values to DRY up the code Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
This commit stops using pointers for a number of structures.
For example: ovsdb.DatabaseSchema, model.DatabaseModel
Pointers are great for values that need to be mutable.
But in all the cases changed, we don't need to mutate these
values.
Also fixes:
deferredUpdates
wasn't cleared on reconnect - only when a client properly shuts down the connectionWithReconnect(500 * time.Milliseconds)
is no longer long enough to process the initial cache + any deferred updates in CI - slow machine - for a database with approximately 10 bridges 😱