StateDB: split write methods from Table into RWTable#28140
StateDB: split write methods from Table into RWTable#28140joamaki merged 4 commits intocilium:mainfrom
Conversation
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
joamaki
force-pushed
the
pr/joamaki/statedb-rwtable
branch
from
cfa64f2 to
692d641
Compare
joamaki
force-pushed
the
pr/joamaki/statedb-rwtable
branch
from
692d641 to
1ef9845
Compare
joamaki
added
the
release-note/misc
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
joamaki
changed the title
|
/test |
joamaki
requested review from
bimmlerd
and removed request for
dylandreimerink
giorio94
left a comment
giorio94
left a comment
There was a problem hiding this comment.
The overall logic looks good to me, just a couple of minor comments inline.
Is RWTable a good name? What about TableWriter? Should Table[Obj] be RTable[Obj] or TableReader[Obj]? WDYT?
Personally, I like TableReader and TableReadWriter, but that's fairly verbose. RWTable and RTable also sound good to me (and seem more symmetric than just plain Table). The main issue with just TableWriter is that it actually also provides the read functionalities.
WriteTxn still takes a TableMeta (from Table[Obj]). Reasoning is that we want to be able to construct a WriteTxn against multiple tables (for that cross-table atomic transaction sweetness) even when RWTable isn't directly available (but indirect smart writers are). An alternative I see to this is some sort of WriteTxnBuilder pattern to allow composing together the WriteTxn in a controlled way, e.g. a smart writer would take WriteTxnBuilder and then return (WriterHandle, WriteTxnBuilder). This way one explicitly asks for access to the writer methods, but I'm not sure if this boilerplate brings a lot of value. The downside of allowing WriteTxn with Table[Obj] is that arbitrary code can lock the table without intention or ability to write to it, but I would expect this to be easily catched by code review.
I'm personally fine with keeping the current structure for the moment, and perhaps re-evaluate if more of such usages start to pop up. As you mentioned it seems pretty easy to catch incorrect creations of write transactions on read-only tables.
bimmlerd
left a comment
bimmlerd
left a comment
There was a problem hiding this comment.
I'm afraid of the footgun in the DeleteTracker API.
joamaki
force-pushed
the
pr/joamaki/statedb-rwtable
branch
from
1ef9845 to
987fc7d
Compare
|
/test |
joamaki
force-pushed
the
pr/joamaki/statedb-rwtable
branch
from
987fc7d to
ea9d30a
Compare
bimmlerd
left a comment
bimmlerd
left a comment
There was a problem hiding this comment.
Yep I like this much better. Confused about the Delete semantics currently, other than that looks good!
joamaki
force-pushed
the
pr/joamaki/statedb-rwtable
branch
from
ea9d30a to
ad97b4e
Compare
bimmlerd
left a comment
bimmlerd
left a comment
There was a problem hiding this comment.
two nits, after that I'm happy!
To allow control over where a table can be modified, split the Table[Obj] interface into reader (Table[Obj]) and writer (RWTable[Obj]) parts. RWTable[Obj] is a superset of Table[Obj], and can thus be used everywhere where a Table[Obj] is required. For convenience, implement NewProtectedTableCell() to provide the RWTable[Obj] only in the hive module's scope, and Table[Obj] globally. With this, the cell defining the table can choose to either not allow modifications at all to the table (tests can still go via NewTable() to create a modifiable table), or only allow modifications under very controller and validated ways. Signed-off-by: Jussi Maki <[email protected]> Refresh of statedb-split-write-methods
Make RWTable[*Device] and RWTable[*Route] private to the devices controller to make sure they're not modified by anyone else. The DeviceTableCell and RouteTableCell are still available in the tables package for tests to use. Signed-off-by: Jussi Maki <[email protected]>
These allow efficiently doing optimistic concurrency control in
cases where a long-running computation is performed against the
data and then results written back to a table, e.g.
1. With ReadTxn compute results for each object (with revision number)
2. With WriteTxn commit the results using CompareAndDelete. If the
object has changed, the stale result for that object is ignored.
3. Wait for changes and repeat
Signed-off-by: Jussi Maki <[email protected]>
Benchmark the delay and throughput for propagating changes from one table
to another. This simulates the expected use-case for statedb where we would
be reading from one or more tables and computing desired state that is written
to another table.
Results on my machine:
goos: linux
goarch: amd64
pkg: github.com/cilium/cilium/pkg/statedb
cpu: AMD Ryzen 9 5950X 16-Core Processor
BenchmarkDB_PropagationDelay 224287 5071 ns/op
--- BENCH: BenchmarkDB_PropagationDelay
benchmarks_test.go:349: Propagation delay (N: 224287) 50p: 37.12µs, 90p: 91.633µs, 99p: 123.924µs
On a single core, 197k objects per second with batch size of 10. For batch size of 1
I got 61k objects per second, so even small increase in batch size oversets the
cost of waiting for the watch channel to close. Propagating the batch
took ~123 microseconds (99 percentile).
Signed-off-by: Jussi Maki <[email protected]>
joamaki
force-pushed
the
pr/joamaki/statedb-rwtable
branch
from
ad97b4e to
7763aad
Compare
|
/test |
maintainer-s-little-helper
bot
added
the
ready-to-merge
4 participants
NewProtectedTableCell. This allows modules to define protected tables where outside modules can only read from the table, or write to the table only in restricted/validated ways.Open questions:
RWTablea good name? What aboutTableWriter? ShouldTable[Obj]beRTable[Obj]orTableReader[Obj]? WDYT?Table[Obj]). Reasoning is that we want to be able to construct aWriteTxnagainst multiple tables (for that cross-table atomic transaction sweetness) even whenRWTableisn't directly available (but indirect smart writers are). An alternative I see to this is some sort ofWriteTxnBuilderpattern to allow composing together theWriteTxnin a controlled way, e.g. a smart writer would takeWriteTxnBuilderand then return(WriterHandle, WriteTxnBuilder). This way one explicitly asks for access to the writer methods, but I'm not sure if this boilerplate brings a lot of value. The downside of allowingWriteTxnwithTable[Obj]is that arbitrary code can lock the table without intention or ability to write to it, but I would expect this to be easily catched by code review.