[RFC] client: honor last transaction IDs

[dcbw]

Maybe fixes #250

@dave-tucker is this kinda what you were thinking of, albeit not very clean?

[coveralls]

Pull Request Test Coverage Report for Build 1790135334

• 49 of 81 (60.49%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.09%) to 73.522%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
util/errors.go	5	11	45.45%
cache/cache.go	14	27	51.85%
client/client.go	30	43	69.77%

Totals
Change from base Build 1754146115:	0.09%
Covered Lines:	4465
Relevant Lines:	6073

---

💛 - Coveralls

[dave-tucker]

This is a step in the right direction...

The missing piece is here:

libovsdb/client/client.go

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.

[dave-tucker]

I think we should move this to the monitor() function...

[dave-tucker]

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.

[dave-tucker]

@dcbw there are also some edge cases that we might need to document for... (or have the API prevent)

1. What happens if we have 2 monitors on the same table?
2. What happens if we have 2 conditional monitors on the same table with different conditions
3. What happens if we have 2 conditional monitors on the same table with overlapping conditions

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 found == false and the cache would be purged and rebuilt... and we'd be eventually consistent.

[dave-tucker]

Oh also, you'll want to reset lastTransactioId to be empty if we do clear the cache...

[dcbw]

@dave-tucker I thought I tried to account for these:

• What happens if we have 2 monitors on the same table?

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.

• What happens if we have 2 conditional monitors on the same table with different conditions

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.

• What happens if we have 2 conditional monitors on the same table with overlapping 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.

[dcbw]

Thought about this more. Do I have the stuff backwards? Is the following correct?

1. for CondSince we want to purge everything except rows that match any of the conditions. Becuase when we get the monitor reply we'll get updates for those rows based on their current state (which is the LastTransID).

2. But for !Since we do want to purge everything since we'll get full updates of current state for the monitored rows so we can purge them.

And just to confirm; we are OK purging rows that dont' match any of the monitors/conditions?

[dave-tucker]

@dcbw

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.

Right, the trouble then becomes how you clear the cache if ovsdb-server says "i don't have that lastTransactionID".
We'd probably need to disconnect/reconnect and start from scratch across all monitors I guess...

Thought about this more. Do I have the stuff backwards? Is the following correct?

1. for CondSince we want to purge everything except rows that match any of the conditions. Becuase when we get the monitor reply we'll get updates for those rows based on their current state (which is the LastTransID).

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 found = false so we'd need to drop everything. It's this behaviour that makes me think we need to have a per-monitor purge() function that clears ONLY the tables/rows that it's monitoring.

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 cache.Populate3 which does as cache.Populate2 but also propagates the found bool.
And we'd just have the cache overwrite everything without any existence checks if found=false

2. But for !Since we do want to purge everything since we'll get full updates of current state for the monitored rows so we can purge them.

This is correct.

And just to confirm; we are OK purging rows that dont' match any of the monitors/conditions?

Yep. It's essentially a noop so we could skip it.