add util for transaction table #528
Conversation
k8s-ci-robot
added
cncf-cla: yes
freehan
force-pushed
the
transaction-table
branch
from
029ce08
to
5660ec0
Compare
pkg/neg/syncers/transaction_table.go
Outdated
| detachTransaction = transactionOperation("DETACH") | ||
| ) | ||
|
|
||
| type transactionOperation string |
There was a problem hiding this comment.
can this be int consts, e.g. with iota ? https://splice.com/blog/iota-elegant-constants-golang/
There was a problem hiding this comment.
The transaction operation string will be used in error message though.
There was a problem hiding this comment.
If you want, you could use int costs w/ iota and then implement a ToString() function.
|
|
||
| // Insert entries into transaction table | ||
| for i := 0; i < testNum; i++ { | ||
| key := fmt.Sprintf("%s%d", keyPrefix, i) |
There was a problem hiding this comment.
nit - can we use values that look more like real values? I'm assuming the real values are NEG names
There was a problem hiding this comment.
There are test on the user side of the transaction table that does this. This unit test focused on the util.
pkg/neg/syncers/transaction_table.go
Outdated
| // It uses the encoded endpoint as key and associate attributes of an transaction as value | ||
| type transactionTable struct { | ||
| data map[string]transactionEntry | ||
| lock sync.Mutex |
There was a problem hiding this comment.
golint is angry because it wants this to be *sync.Mutex
There was a problem hiding this comment.
I am removing as golint is angry as the syncer using this util also uses another mutex
| keyPrefix := "key" | ||
| zonePrefix := "zone" | ||
| testKeyMap := map[string]transactionEntry{} | ||
| verifyTable := func() { |
There was a problem hiding this comment.
I feel like it'd be cleaner and more usable for future tests if this were its own function, e.g. func verifyTable(t *testing.T, expectedKeys []string). But since we're only using it in this test case, this can be a later change - up to you
|
|
||
| verifyTable() | ||
|
|
||
| // Update half of the entries in the transaction table |
There was a problem hiding this comment.
Can we have tests for detachTransaction also?
freehan
force-pushed
the
transaction-table
branch
from
5660ec0
to
b16e3fd
Compare
|
Ready for another round. |
|
Ping |
|
|
||
| // transactionTable records ongoing NEG API operation per endpoint | ||
| // It uses the encoded endpoint as key and associate attributes of an transaction as value | ||
| // WARNING: transactionTable is not thread safe |
There was a problem hiding this comment.
Based on our offline discussion, you need to make this thread-safe right?
There was a problem hiding this comment.
The user of transaction table will guarantee thread-safty.
I remove the RWlock in the util mostly because golint complains if you have a mutex wrap around another mutex.
pkg/neg/syncers/transaction_table.go
Outdated
| detachTransaction = transactionOperation("DETACH") | ||
| ) | ||
|
|
||
| type transactionOperation string |
There was a problem hiding this comment.
If you want, you could use int costs w/ iota and then implement a ToString() function.
freehan
force-pushed
the
transaction-table
branch
2 times, most recently
from
90276d5
to
6ddf88a
Compare
|
Fixed the comments |
|
Ready for another round |
pkg/neg/syncers/transaction_table.go
Outdated
| attachTransaction = iota | ||
| detachTransaction | ||
| attachOpertaion = "Attach" | ||
| detachOpertaion = "Detach" |
There was a problem hiding this comment.
Nit: Fix spelling of "operation"
pkg/neg/syncers/transaction_table.go
Outdated
| const ( | ||
| attachTransaction = iota | ||
| detachTransaction | ||
| attachOpertaion = "Attach" |
There was a problem hiding this comment.
I don't think you need an extra const here
Just inline "Attach" in String()
pkg/neg/syncers/transaction_table.go
Outdated
| package syncers | ||
|
|
||
| const ( | ||
| attachTransaction = iota |
There was a problem hiding this comment.
I would call this attachOp. Same for detach
pkg/neg/syncers/transaction_table.go
Outdated
| unknownOperation = "UnknownOperation" | ||
| ) | ||
|
|
||
| type transactionOperation int |
There was a problem hiding this comment.
For brevity, maybe just call this transactionOp
freehan
force-pushed
the
transaction-table
branch
from
6ddf88a
to
ae0caed
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: freehan, rramkumar1 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
util needed for transaction-syncer
cc @agau4779