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

*: sort by ASCEND on missing sort order #6672

Merged
merged 4 commits into from
Oct 19, 2016
Merged

*: sort by ASCEND on missing sort order #6672

merged 4 commits into from
Oct 19, 2016

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Oct 18, 2016 •
edited
Loading

Address #6671.

/cc @xiang90

xiang90
xiang90 reviewed Oct 18, 2016
View reviewed changes
etcdctl/ctlv3/command/get_command.go
@@ -105,6 +106,13 @@ func getGetOp(cmd *cobra.Command, args []string) (string, []clientv3.OpOption) {
opts = append(opts, clientv3.WithRev(getRev))
}

if getSortOrder == "" && getSortTarget != "" {
ExitWithError(ExitBadFeature, errors.New("missing sort order (--order)"))
Copy link
Contributor

Choose a reason for hiding this comment

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

exitbadflag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have bad flag exit. Instead used ExitBadArgs. Is that ok?

PTAL.

@xiang90
Copy link
Contributor

xiang90 commented Oct 18, 2016

If there is no order given, shouldnt etcd assumes an ordering?

@xiang90
Copy link
Contributor

xiang90 commented Oct 18, 2016

I do not know why i tell etcdctl to sort by create revision but it does not.

@gyuho
Copy link
Contributor Author

gyuho commented Oct 18, 2016

@xiang90 WithSort does

func WithSort(target SortTarget, order SortOrder) OpOption {
    return func(op *Op) {
        if target == SortByKey && order == SortAscend {
            // If order != SortNone, server fetches the entire key-space,
            // and then applies the sort and limit, if provided.
            // Since current mvcc.Range implementation returns results
            // sorted by keys in lexiographically ascending order,
            // client should ignore SortOrder if the target is SortByKey.
            order = SortNone
        }
        op.sort = &SortOption{target, order}
    }
}

But this doesn't cover the case of sort-by=CREATE.

It works when sort-by=KEY because it returns lexiographically ascending order by default.

@xiang90
Copy link
Contributor

xiang90 commented Oct 18, 2016

We should fill in a default order for users when they specify sort-by x when x is not key.

@gyuho
Copy link
Contributor Author

gyuho commented Oct 18, 2016

@xiang90 Right that's better. Will rework on this with more tests. Thanks.

@gyuho gyuho changed the title ctlv3: error on --order or --sort-by missing ctlv3: default sort order on missing sort target Oct 18, 2016
xiang90
xiang90 reviewed Oct 18, 2016
View reviewed changes
etcdctl/ctlv3/command/get_command.go
@@ -137,6 +137,13 @@ func getGetOp(cmd *cobra.Command, args []string) (string, []clientv3.OpOption) {
ExitWithError(ExitBadFeature, fmt.Errorf("bad sort target %v", getSortTarget))
}

if sortByTarget != clientv3.SortByKey && sortByOrder == clientv3.SortNone {
// Since current mvcc.Range implementation returns results
Copy link
Contributor

Choose a reason for hiding this comment

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

alternatively, can we fix this at server side? in apply.go line 321?

Copy link
Contributor

Choose a reason for hiding this comment

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

if any target is specified and sort_order == none, we should set sort order to ascend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure will try that.

@gyuho gyuho changed the title ctlv3: default sort order on missing sort target *: default sort order on missing sort target Oct 18, 2016
@gyuho gyuho changed the title *: default sort order on missing sort target *: sort by ASCEND on missing sort order Oct 18, 2016
@gyuho gyuho force-pushed the etcdctl-sort-by branch 2 times, most recently from 17fb4b7 to 41aad54 Compare October 18, 2016 23:09
xiang90
xiang90 reviewed Oct 18, 2016
View reviewed changes
clientv3/op.go
@@ -227,6 +227,8 @@ func WithSort(target SortTarget, order SortOrder) OpOption {
// client should ignore SortOrder if the target is SortByKey.
order = SortNone
}
// server-side sets order == SortAscend by default
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok removed.

xiang90
xiang90 reviewed Oct 18, 2016
View reviewed changes
etcdserver/apply.go
@@ -323,6 +323,11 @@ func (a *applierV3backend) Range(txnID int64, r *pb.RangeRequest) (*pb.RangeResp
sort.Sort(sorter)
case r.SortOrder == pb.RangeRequest_DESCEND:
sort.Sort(sort.Reverse(sorter))
case r.SortTarget != pb.RangeRequest_KEY:
Copy link
Contributor

Choose a reason for hiding this comment

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

i do not understand this. we will only reach this code if sort order is not none (see 307). What we want is to sort the keys if target == create and order == none.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, will fix.

@gyuho gyuho force-pushed the etcdctl-sort-by branch 2 times, most recently from de21c8c to c123f7e Compare October 18, 2016 23:28
xiang90
xiang90 reviewed Oct 18, 2016
View reviewed changes
clientv3/integration/kv_test.go
@@ -226,6 +226,21 @@ func TestKVRange(t *testing.T) {
{Key: []byte("fop"), Value: nil, CreateRevision: 9, ModRevision: 9, Version: 1},
},
},
// range all with SortByKey, missing sorting order (ASCEND by default)
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably should also add a server side test.

@xiang90
Copy link
Contributor

xiang90 commented Oct 18, 2016

LGTM

@gyuho
Copy link
Contributor Author

gyuho commented Oct 18, 2016

@xiang90 Also added a test case in server side. Will merge if there's no objection? Thanks.

@xiang90
Copy link
Contributor

xiang90 commented Oct 18, 2016

LGTM

@gyuho gyuho mentioned this pull request Oct 18, 2016
Closed
@gyuho gyuho merged commit c9b7fc4 into etcd-io:master Oct 19, 2016
@gyuho gyuho deleted the etcdctl-sort-by branch October 19, 2016 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiang90 xiang90 xiang90 left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gyuho @xiang90