server/region_handler: txn debug tool for tikv #3787
Conversation
| startKey = codec.EncodeBytes(nil, tableStartKey) | ||
| endKey = codec.EncodeBytes(nil, tableEndKey) | ||
| startKey = EncodeRowKeyWithHandle(tableID, math.MinInt64) | ||
| endKey = EncodeRowKeyWithHandle(tableID, math.MaxInt64) |
There was a problem hiding this comment.
Why do you need to update table codec?
There was a problem hiding this comment.
Since when use a codecPDClient instead of pd.Client in regionHandler, there is no need to use codec.EncodeBytes again. And this function will only be used in regionHandler
store/tikv/mock-tikv/mvcc.go
Outdated
| info.Writes = make([]*kvrpcpb.WriteInfo, len(e.values), len(e.values)) | ||
| info.Values = make([]*kvrpcpb.ValueInfo, len(e.values), len(e.values)) | ||
|
|
||
| for id, item := range e.values { |
store/tikv/mock-tikv/mvcc.go
Outdated
| } | ||
| } | ||
|
|
||
| info.Writes = make([]*kvrpcpb.WriteInfo, len(e.values), len(e.values)) |
There was a problem hiding this comment.
info.Writes = make([]*kvrpcpb.WriteInfo, len(e.values))
| tp = kvrpcpb.Op_Del | ||
| case typeRollback: | ||
| tp = kvrpcpb.Op_Rollback | ||
| } |
There was a problem hiding this comment.
I think it's more clear to list all values in switch-case:
var tp mvccValueType
switch item.valueType {
case typePut:
case typeDelete:
case typeRollback:
}
There was a problem hiding this comment.
address comment
store/tikv/mock-tikv/mvcc.go
Outdated
| @@ -547,6 +598,35 @@ func (s *MvccStore) RawScan(startKey, endKey []byte, limit int) []Pair { | |||
| return pairs | |||
| } | |||
|
|
|||
| // MvccGetByStartTS gets mvcc info for the primary key with startTS | |||
| func (s MvccStore) MvccGetByStartTS(starTS uint64) *kvrpcpb.MvccInfo { | |||
There was a problem hiding this comment.
I think you need to add startKey and endKey arguments to restrict the method in a single region.
server/region_handler_test.go
Outdated
| "encoding/json" | ||
| "fmt" | ||
| "math" | ||
| "net/http" | ||
|
|
||
| "bytes" |
server/region_handler.go
Outdated
|
|
||
| const ( | ||
| opMvccGetByKey = "key" | ||
| opMvccGetByTXN = "txn" |
There was a problem hiding this comment.
We usually use Txn instead of TXN.
server/region_handler.go
Outdated
| } | ||
|
|
||
| func (rh TableRegionsHandler) getRegionsMeta(regionIDs []uint64) ([]RegionMeta, error) { | ||
| func (tool *regionHandlerTool) getRegionsMeta(regionIDs []uint64) ([]RegionMeta, error) { |
There was a problem hiding this comment.
We usually use t instead of tool.
|
@disksing PTAL |
server/region_handler.go
Outdated
| return rh.getMvccByRecordID(tableID, recordID) | ||
| } | ||
|
|
||
| func (rh *mvccTxnHandler) handleMvccGetByTXN(params map[string]string) (interface{}, error) { |
server/region_handler.go
Outdated
|
|
||
| tikvReq := &tikvrpc.Request{ | ||
| Type: tikvrpc.CmdMvccGetByStartTs, | ||
| Priority: kvrpcpb.CommandPri_Normal, |
There was a problem hiding this comment.
Maybe Low? We don't want debug commands slow down normal ones.
| }, | ||
| } | ||
| kvResp, err := t.store.SendReq(t.bo, tikvReq, curRegion.Region, time.Hour) | ||
| log.Info(startTS, string(startKey), curRegion.Region, string(curRegion.StartKey), string(curRegion.EndKey), kvResp) |
There was a problem hiding this comment.
Forget to remove the debug message?
There was a problem hiding this comment.
No , it's used to show the process imformation @disksing
server/region_handler_test.go
Outdated
| c.Assert(len(data.Info.Writes) > 0, IsTrue) | ||
| startTs := data.Info.Writes[0].StartTs | ||
|
|
||
| resp, err = http.Get(fmt.Sprintf("http://127.0.0.1:10090/mvcc/txn/%d", startTs)) |
There was a problem hiding this comment.
Add a test that the ts doesn't exist.
There was a problem hiding this comment.
address comments
|
LGTM. |
server/region_handler.go
Outdated
| ) | ||
|
|
||
| // prepare checks and prepares for region request. It returns | ||
| // regionHandlerTool on success while return an err on any |
There was a problem hiding this comment.
while returns an error if any error happens.
server/region_handler.go
Outdated
| return | ||
| } | ||
|
|
||
| store := s.driver.(*TiDBDriver).store |
There was a problem hiding this comment.
Frankly speaking, those kind of logic is vulnerable, if the implementation change one day, we got a panic here.
There was a problem hiding this comment.
Any suggestion @tiancaiamao ?
| } | ||
|
|
||
| type regionHandlerTool struct { | ||
| bo *tikv.Backoffer |
There was a problem hiding this comment.
Why store backoffer rather than pass it as argument?
There was a problem hiding this comment.
What do you mean?The regionHanderTool is used to generate and store common tools for each regionHander. I set the backoffer as a member of the tool since more than one regionHander needed it @tiancaiamao
| js, err := json.Marshal(data) | ||
| if err != nil { | ||
| rh.writeError(w, err) | ||
| return | ||
| } | ||
| log.Info(string(js)) |
There was a problem hiding this comment.
It's used only in this debug tool, we needed it. @tiancaiamao
server/http_status.go
Outdated
| router.Handle("/regions/{regionID}", s.newRegionHandler(pdClient)) | ||
|
|
||
| tikvHandler, err := s.newRegionHandler() | ||
| if err == nil { |
There was a problem hiding this comment.
log an error or fatal for error.
There was a problem hiding this comment.
Since the error won't be nil when the store is not tikv, I think it's more appropriate to log an info here @siddontang .
| ts.startServer(c) | ||
| ts.prepareData(c) | ||
| defer ts.stopServer(c) | ||
| resp, err := http.Get(fmt.Sprintf("http://127.0.0.1:10090/mvcc/key/tidb/test/1")) |
There was a problem hiding this comment.
why use a fixed port 10090 here? It is better to listen 0 and get a random port for the test.
There was a problem hiding this comment.
It's referenced on tidb_test.go. And I'm not pretty sure that whether we should keep same with it. @siddontang
server/region_handler.go
Outdated
| if !ok { | ||
| err = fmt.Errorf("Invalid KvStore") | ||
| return | ||
| } |
There was a problem hiding this comment.
I think you shouldn't return a "Invalid KvStore" when driver assert failed.
There was a problem hiding this comment.
any suggestion?
There was a problem hiding this comment.
Maybe like this?
if store, ok := s.driver.(*TiDBDriver); !ok {
err = fmt.Errorf("Invalid Driver")
} else {
if tikvStore, ok = store.store.(kvStore); !ok {
err = fmt.Errorf("Invalid kvStore")
}
}
if err != nil {
return
}
server/region_handler.go
Outdated
| tikvStore, ok = store.store.(kvStore) | ||
| } | ||
| if !ok { | ||
| err = fmt.Errorf("Invalid KvStore") |
server/region_handler_test.go
Outdated
| err = decoder.Decode(&data) | ||
| c.Assert(err, IsNil) | ||
| c.Assert(data.Info, NotNil) | ||
| c.Assert(len(data.Info.Writes) > 0, IsTrue) |
There was a problem hiding this comment.
c.Assert(len(data.Info.Writes), Larger, 0)
server/region_handler.go
Outdated
| tikvStore, ok = store.store.(kvStore) | ||
| } | ||
| if !ok { | ||
| err = errors.New("Invalid KvStore") |
There was a problem hiding this comment.
Different error kind for invalid kvstore and invalid driver
server/region_handler.go
Outdated
| } | ||
|
|
||
| if tikvStore, ok = store.store.(kvStore); !ok { | ||
| log.Fatal("Invalid KvStore with illegal store") |
There was a problem hiding this comment.
Why use log.Fatal instead of returning an error?
There was a problem hiding this comment.
Since this error should never happen. And if it failed there must some unrecoverable disaster happened @choleraehyq
There was a problem hiding this comment.
If so, why not just panic?
There is little reason to use log.Fatal. It call os.Exit behind, which will exit immediately without calling deferred functions, which may lead to resource leaking or something. Also, it doesn't print out the calling stack.
There was a problem hiding this comment.
And the return after log.Fatal is redundent.
Hi,
This PR implement two API for txn debug:
1. Get MvccInfo BY Key
GET /mvcc/key/{db}/{table}/{recordID}2. Get Primary's MvccInfo by start_ts
/mvcc/txn/{startTS}/{db}/{table}//mvcc/txn/{startTS}@disksing @hhkbp2 PTAL