Skip to content
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.

Sign up for GitHub

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

Jump to bottom

server: support http index mvcc interface #5304

Merged
merged 14 commits into from
Dec 12, 2017
Merged

server: support http index mvcc interface #5304

merged 14 commits into from
Dec 12, 2017

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented Dec 5, 2017

@jackysp jackysp added the WIP label Dec 5, 2017
@jackysp jackysp removed the WIP label Dec 6, 2017
@zimulala
Copy link
Contributor

zimulala commented Dec 6, 2017

/run-all-tests

shenli
shenli reviewed Dec 6, 2017
View reviewed changes
server/region_handler.go Outdated
return kvResp.MvccGetByKey, nil
}

func (t *regionHandlerTool) formValue2DatumRow(values url.Values, idxCols []*model.ColumnInfo) ([]types.Datum, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment for the function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

server/region_handler_test.go Outdated
@@ -32,6 +32,7 @@ import (
"github.com/pingcap/tidb/store/tikv/mocktikv"
"github.com/pingcap/tidb/tablecodec"
"github.com/pingcap/tidb/util/codec"
"io"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this before "github.com/xxxxx".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

server/region_handler.go Outdated
@@ -592,8 +608,38 @@ func (rh mvccTxnHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
}
}

func (rh mvccTxnHandler) handleMvccGetByIdx(params map[string]string, values url.Values) (interface{}, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

AndreMouche
AndreMouche reviewed Dec 7, 2017
View reviewed changes
Copy link
Contributor

@AndreMouche AndreMouche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest LGTM

server/region_handler.go Outdated
}

val := vals[0]
if len(val) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the difference between NULL and empty string?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

server/region_handler.go Outdated
func (t *regionHandlerTool) formValue2DatumRow(values url.Values, idxCols []*model.ColumnInfo) ([]types.Datum, error) {
data := make([]types.Datum, len(idxCols))
for i, col := range idxCols {
var d types.Datum
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use data[i] directly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

server/region_handler.go Outdated
if err != nil {
return nil, err
}
keyLocation, err := t.regionCache.LocateKey(t.bo, encodeKey)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Repeated with L676-L693

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

server/region_handler_test.go Outdated
@@ -282,6 +300,46 @@ func (ts *TidbRegionHandlerTestSuite) TestGetMvcc(c *C) {
err = decoder.Decode(&data2)
c.Assert(err, IsNil)
c.Assert(data2, DeepEquals, data)

// tests for normal index key
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we define an independent function to test index key?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

server/region_handler_test.go
c.Assert(err, IsNil)
decodeKeyMvcc(resp.Body, c, true)

// tests for index key which includes null
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any tests for index key that contains an empty string?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@jackysp
Copy link
Member Author

jackysp commented Dec 7, 2017

/run-all-tests

@jackysp
Copy link
Member Author

jackysp commented Dec 7, 2017

PTAL @shenli and @AndreMouche .

@jackysp
Copy link
Member Author

jackysp commented Dec 7, 2017

@disksing

AndreMouche
AndreMouche reviewed Dec 7, 2017
View reviewed changes
Copy link
Contributor

@AndreMouche AndreMouche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zimulala zimulala added all-tests-passed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 9, 2017
zimulala
zimulala reviewed Dec 9, 2017
View reviewed changes
server/region_handler.go Outdated
data[i].SetMysqlJSON(elem)
default:
return nil, errors.NotSupportedf("Not supported datum type %d.", col.Tp)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the function of ConvertTo instead of line908-987?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

server/region_handler.go Outdated
func (t *regionHandlerTool) getMvccByIdxValue(idx table.Index, values url.Values, idxCols []*model.ColumnInfo, handleStr string) (*kvrpcpb.MvccGetByKeyResponse, error) {
idxRow, err := t.formValue2DatumRow(values, idxCols)
if err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add the error trace? Some places have not added in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add error traces for the whole file.

shenli
shenli reviewed Dec 10, 2017
View reviewed changes
server/region_handler.go Outdated
key, err1 = url.QueryUnescape(key)
if err1 != nil {
if err == nil {
err = err1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we return err here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I just follow the style of url package. Fixed.

server/region_handler.go Outdated
value, err1 = url.QueryUnescape(value)
if err1 != nil {
if err == nil {
err = err1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@shenli
Copy link
Member

shenli commented Dec 11, 2017

LGTM

@shenli
Copy link
Member

shenli commented Dec 11, 2017

/run-all-tests

@shenli shenli added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 11, 2017
@zimulala zimulala merged commit 2cdf05f into pingcap:master Dec 12, 2017
@jackysp jackysp deleted the mvcc_index branch January 4, 2018 09:18
jackysp added a commit to jackysp/tidb that referenced this pull request Jan 12, 2018
@jackysp
server: support http index mvcc interface (pingcap#5304)
9a2bb12
ngaut pushed a commit that referenced this pull request Jan 14, 2018
@jackysp @ngaut
server: schema info and mvcc index APIs for http status server (#5256) (
f79d551 
#5629)

* server: schema info api of http status server (#5256)

* server: support http index mvcc interface (#5304)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndreMouche AndreMouche AndreMouche left review comments

@shenli shenli shenli left review comments

@zimulala zimulala zimulala approved these changes

Assignees
No one assigned
Labels
status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jackysp @zimulala @shenli @AndreMouche