openark
forget operations support removing from kv
Related issue: #966
Description: Support for removing data from KV stores for different forget operations(ForgetInstance, ForgetCluster, ForgetLongUnseenInstances, ForgetUnseenInstancesDifferentlyResolved).
Sorry about the delay; I expect to look into & merge a bunch of community PRs during the first two weeks of October.
Hello @shlomi-noach how are you? :) Is there anything I should do in order to proceed with this PR?
@MaxFedotov thanks for your patience! So, I've been banging my head around this.
On one hand, the essence of this PR makes perfect sense. On the other hand, there is a destructive aspect to it. I'm trying to think whether stale KV entries should be kept around. e.g. the fact orchestrator forgot about an instance doesn't necessarily mean the instance does not exist anymore. Since external apps will depend on KV, I can't anticipate what assumptions those apps will make about existing entries.
For these reasons I find it difficult to approve the PR.
@MaxFedotov thanks for your patience! So, I've been banging my head around this.
On one hand, the essence of this PR makes perfect sense. On the other hand, there is a destructive aspect to it. I'm trying to think whether stale KV entries should be kept around. e.g. the fact
orchestratorforgot about an instance doesn't necessarily mean the instance does not exist anymore. Since external apps will depend on KV, I can't anticipate what assumptions those apps will make about existing entries.For these reasons I find it difficult to approve the PR.
Yeah, but on the other hand, if Orchestrator forgets the instance and it is still in KV, from which external tools get routing data - that will also lead to inconsistencies, e.g. routing to old\stale servers :)
What do you think about adding a configurable parameter to config.go, with the name like RemoveForgottenClustersFromKV, so everyone can decide for each own if they need this or not.
What do you think about adding a configurable parameter to config.go, with the name like RemoveForgottenClustersFromKV, so everyone can decide for each own if they need this or not.
@MaxFedotov sounds good to me!
@shlomi-noach, so i've added RemoveForgottenClustersFromKV config option, fixed a bug with rlike in DeleteRecursive function for internal KV and updated the docs :) And tested everything one more time
@shlomi-noach ping :) Is there anything i need to do in order to proceed with this PR?
type KVStore interface {
PutKeyValue(key string, value string) (err error)
PutKVPairs(kvPairs []*KVPair) (err error)
GetKeyValue(key string) (value string, found bool, err error)
+ DeleteRecursive(key string) (err error)
DistributePairs(kvPairs [](*KVPair)) (err error)
}
[DEBUG] Build only; no packaging
[DEBUG] Building via go version go1.17.6 linux/amd64
go build: -i flag is deprecated
# github.com/openark/orchestrator/go/kv
go/kv/consul_txn.go:104:2: cannot use store (type *consulTxnStore) as type KVStore in return argument:
*consulTxnStore does not implement KVStore (missing DeleteRecursive method)
Hi, @MaxFedotov, it seems that go/kv/consul_txn.go does not implemented DeleteRecursive(key string) (err error), I don't know about Consul transaction store, can you refine this PR further?
Can I simply copy the same code from consulStore.DeleteRecursive to consulTxnStore.DeleteRecursive?
Like:
func (this *consulTxnStore) DeleteRecursive(key string) (err error) {
if this.client == nil {
return nil
}
_, err = this.client.KV().DeleteTree(key, nil)
return err
}
