-
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
[RFC] client: honor last transaction IDs #283
base: main
Are you sure you want to change the base?
Conversation
fc527d8
to
fa20454
Compare
Pull Request Test Coverage Report for Build 1790135334
💛 - Coveralls |
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.
This is a step in the right direction...
The missing piece is here:
Line 859 in 22bd5be
if err == nil && reply.Found { |
You need to purge the cache only if reply.found != true
, otherwise the results from the monitor RPC can be populated on top of the cache as-is (since it was correct up to lastTransactionId). To do this, we probably need to move purge()
to happen inside this function.
client/client.go
Outdated
@@ -243,6 +295,10 @@ func (o *ovsdbClient) connect(ctx context.Context, reconnect bool) error { | |||
for dbName, db := range o.databases { | |||
db.monitorsMutex.Lock() | |||
defer db.monitorsMutex.Unlock() | |||
|
|||
if err := db.purge(dbName); err != nil { |
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 think we should move this to the monitor()
function...
client/client.go
Outdated
@@ -199,6 +200,65 @@ func (o *ovsdbClient) Connect(ctx context.Context) error { | |||
return nil | |||
} | |||
|
|||
func (db *database) purge(name string) error { |
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.
Yes this is along the lines of what I had in mind, but I think we can simplify...
Have monitor()
call this function while reconnecting (it already has reconnecting bool
) to put the cache in the correct state. It will need to be re-written to be scoped only to the monitor that is calling it... this would happen after we get the RPC reply but before we populate updates.
@dcbw there are also some edge cases that we might need to document for... (or have the API prevent)
In all likelihood I think we'll be fine as I would hope that we'd get consistent results from ovsdb-server (as in both monitors would get |
Oh also, you'll want to reset |
@dave-tucker I thought I tried to account for these:
purge() tries to handle this by building up the map of tables to purge. If any of the monitors are not CondSince then it purges the entire table.
purge() adds all the conditions from all monitors for this table to a list and purges the cache of all rows that match any of the conditions.
Would that work different from the second case? If they have overlapping conditions, don't we just want to blow away any row that matches any of the conditions of any monitors on the table? In any case, I'm not sure we can do this all in monitor() itself, since that deals with just a single monitor. But we really need a view of all monitors to know how to purge each table and/or its rows. |
Thought about this more. Do I have the stuff backwards? Is the following correct?
And just to confirm; we are OK purging rows that dont' match any of the monitors/conditions? |
Right, the trouble then becomes how you clear the cache if ovsdb-server says "i don't have that lastTransactionID".
Partially correct. When we get the monitor reply we MAY get updates for those rows based on updates since the lastTransactioID. It's also possible that we get the full db state if In the case of overlapping monitor tables/conditions we'd end up doing multiple purges... but the end state would always be the same... although I honestly think that overlapping monitor conditions is a terrible idea and we should just plain up state that it's not supported. The only other option I can see is to have a
This is correct.
Yep. It's essentially a |
Signed-off-by: Dan Williams <dcbw@redhat.com>
fa20454
to
330edf0
Compare
Maybe fixes #250
@dave-tucker is this kinda what you were thinking of, albeit not very clean?