-
Notifications
You must be signed in to change notification settings - Fork 92
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
DB Access Layer Merges: GetTablePattern ... #103
Conversation
// Run Lua script [Found = SUCCESS return] | ||
if d.Opts.IsWriteDisabled && !exists { | ||
|
||
//glog.Info("ExistKeysPattern: B4= ", luaScriptExistsKeysPatterns.Hash()) |
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.
Can we remove this and L137?
`) | ||
|
||
// Alternate Lua Script | ||
// luaScriptExistsKeysPatterns = redis.NewScript(` |
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 part looks redundant to me. If the register in L184 executed, all keys should have registered LUA. Is there any case we may need register for single key?
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.
It is a comment, to indicate an alternate implementation. I would rather keep it.
|
||
keys = make([]Key, 0, len(tkNv)/2) | ||
for i, v := range tkNv { | ||
// glog.Info("GetTablePattern: i: ", i, " v: ", v) |
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.
If this is not needed, please clean it up.
return table, err | ||
} | ||
} else { | ||
// glog.Info("GetTablePattern: i: ", i, " v: ", v) |
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.
Same. Clean up.
if cTable, ok := d.cache.Tables[ts.Name]; ok && cTable.complete { | ||
|
||
// Copy relevent keys of cTable to table, and set keys | ||
// Be aware of cache poisoning |
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 am curious if the code handles the cache poisoning case.
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, it is handled by the Copy() on L113
delete(tOrig.patterns, db.key2redis(&ts, Key{Comp: []string{"*"}})) | ||
|
||
if !reflect.DeepEqual(tPat, tOrig) { | ||
fmt.Println("\ntPat: \n", tPat) |
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.
Why not t.Logf()
} | ||
|
||
if !reflect.DeepEqual(tPat, tOrig) { | ||
fmt.Println("\ntPat: \n", tPat) |
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.
Same. Suggest to replace fmt.Println() to t.Logf()
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.
A review has been raised for fixing the issues raised in the review comments:
#106
Added the following APIs: ExistKeysPattern() GetTablePattern() Key.IsAllKeyPattern() Go UT, and Benchmark Tests for the same TranslibDBScriptFail error type to translib/tlerr package
Added the following APIs:
ExistKeysPattern()
GetTablePattern()
Key.IsAllKeyPattern()
Go UT, and Benchmark Tests for the same
TranslibDBScriptFail error type to translib/tlerr package