You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2016/11/15 19:55:57 UTC

[kudu-CR] [Timestamp] use operator '<' instead of ComesBefore

Alexey Serbin has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/5096

Change subject: [Timestamp] use operator '<' instead of ComesBefore
......................................................................

[Timestamp] use operator '<' instead of ComesBefore

Added more syntactic sugar, now for the kudu::Timestamp class.
Also, did a minor clean-up on the set of header files and forward
declarations.

This patch does not contain any functional changes.

Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
---
M src/kudu/common/timestamp.cc
M src/kudu/common/timestamp.h
M src/kudu/tablet/compaction.h
3 files changed, 48 insertions(+), 29 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/5096/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>

[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: [Timestamp] use 'operator<' instead of ComesBefore
......................................................................


Patch Set 3:

(4 comments)

> (4 comments)
 > 
 > lgtm, but please remove the CompactionInputRow stuff

Sure -- will post an update soon.

http://gerrit.cloudera.org:8080/#/c/5096/3/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:

PS3, Line 474: 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
> same
Done


PS3, Line 350: if (smallest < state->next()) {
> same
Done


PS3, Line 565: namespace {
             : void AdvanceToLastInList(const Mutation** m) {
             :   const Mutation* next;
             :   while ((next = (*m)->acquire_next()) != nullptr) {
             :     *m = next;
             :   }
             : }
             : } // anonymous namespace
             : 
             : // Compare the mutations of two duplicated rows.
             : // Return 'true' if latest_version(left) < latest_version(right)
             : bool operator<(const CompactionInputRow& lhs, const CompactionInputRow& rhs) {
             :   const Mutation* left_latest = lhs.redo_head;
             :   const Mutation* right_latest = rhs.redo_head;
             :   if (right_latest == nullptr) {
             :     DCHECK(left_latest != nullptr);
             :     return true;
             :   }
             :   if (left_latest == nullptr) {
             :     // left must still be alive
             :     DCHECK(right_latest != nullptr);
             :     return false;
             :   }
             : 
             :   // Duplicated rows have disjoint histories, we don't need to get the latest
             :   // mutation, the first one should be enough for the sake of determining the most recent
             :   // row, but in debug mode do get the latest to make sure one of the rows is a ghost.
             :   const bool ret = left_latest->timestamp() < right_latest->timestamp();
             : #ifndef NDEBUG
             :   AdvanceToLastInList(&left_latest);
             :   AdvanceToLastInList(&right_latest);
             :   if (left_latest->timestamp() != right_latest->timestamp()) {
             :     // If in fact both rows were deleted at the same time, this is OK -- we could
             :     // have a case like TestRandomAccess.TestFuzz3, in which a single batch
             :     // DELETED from the DRS, INSERTed into MRS, and DELETED from MRS. In that case,
             :     // the timestamp of the last REDO will be the same and we can pick whichever
             :     // we like.
             :     const bool debug_ret = left_latest->timestamp() < right_latest->timestamp();
             :     CHECK_EQ(ret, debug_ret);
             :   }
             : #endif
             :   return ret;
             : }
> same
Done


http://gerrit.cloudera.org:8080/#/c/5096/3/src/kudu/tablet/compaction.h
File src/kudu/tablet/compaction.h:

PS3, Line 167: bool operator<(const CompactionInputRow& lhs, const CompactionInputRow& rhs);
> this seems out of place in this patch (the commit message only mentions Tim
Sure, especially if it's about to conflict with your patch.  I just wanted to get rid of most of ComesBefore() and CompareTo().


-- 
To view, visit http://gerrit.cloudera.org:8080/5096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has submitted this change and it was merged.

Change subject: [Timestamp] use 'operator<' instead of ComesBefore
......................................................................


[Timestamp] use 'operator<' instead of ComesBefore

Added more syntactic sugar, now for the kudu::Timestamp class:
use operator{<, <=, >, <=, ==} instead of Timestamp::ComesBefore()
and Timestamp::CompareTo(), where possible.
Besides, this patch contains a minor clean-up on the of header files
and forward declarations.

This patch does not contain any functional changes.

Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Reviewed-on: http://gerrit.cloudera.org:8080/5096
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves <dr...@apache.org>
---
M src/kudu/common/timestamp.cc
M src/kudu/common/timestamp.h
M src/kudu/server/hybrid_clock.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/delta_stats.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tserver/tablet_service.cc
9 files changed, 75 insertions(+), 54 deletions(-)

Approvals:
  David Ribeiro Alves: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/5096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Dinesh Bhat, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5096

to look at the new patch set (#5).

Change subject: [Timestamp] use 'operator<' instead of ComesBefore
......................................................................

[Timestamp] use 'operator<' instead of ComesBefore

Added more syntactic sugar, now for the kudu::Timestamp class:
use operator{<, <=, >, <=, ==} instead of Timestamp::ComesBefore()
and Timestamp::CompareTo(), where possible.
Besides, this patch contains a minor clean-up on the of header files
and forward declarations.

This patch does not contain any functional changes.

Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
---
M src/kudu/common/timestamp.cc
M src/kudu/common/timestamp.h
M src/kudu/server/hybrid_clock.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/delta_stats.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tserver/tablet_service.cc
9 files changed, 75 insertions(+), 54 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/5096/5
-- 
To view, visit http://gerrit.cloudera.org:8080/5096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: [Timestamp] use 'operator<' instead of ComesBefore
......................................................................


Patch Set 3:

(4 comments)

lgtm, but please remove the CompactionInputRow stuff

http://gerrit.cloudera.org:8080/#/c/5096/3/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:

PS3, Line 474: 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
             : 
same


PS3, Line 350: if (smallest < state->next()) {
same


PS3, Line 565: namespace {
             : void AdvanceToLastInList(const Mutation** m) {
             :   const Mutation* next;
             :   while ((next = (*m)->acquire_next()) != nullptr) {
             :     *m = next;
             :   }
             : }
             : } // anonymous namespace
             : 
             : // Compare the mutations of two duplicated rows.
             : // Return 'true' if latest_version(left) < latest_version(right)
             : bool operator<(const CompactionInputRow& lhs, const CompactionInputRow& rhs) {
             :   const Mutation* left_latest = lhs.redo_head;
             :   const Mutation* right_latest = rhs.redo_head;
             :   if (right_latest == nullptr) {
             :     DCHECK(left_latest != nullptr);
             :     return true;
             :   }
             :   if (left_latest == nullptr) {
             :     // left must still be alive
             :     DCHECK(right_latest != nullptr);
             :     return false;
             :   }
             : 
             :   // Duplicated rows have disjoint histories, we don't need to get the latest
             :   // mutation, the first one should be enough for the sake of determining the most recent
             :   // row, but in debug mode do get the latest to make sure one of the rows is a ghost.
             :   const bool ret = left_latest->timestamp() < right_latest->timestamp();
             : #ifndef NDEBUG
             :   AdvanceToLastInList(&left_latest);
             :   AdvanceToLastInList(&right_latest);
             :   if (left_latest->timestamp() != right_latest->timestamp()) {
             :     // If in fact both rows were deleted at the same time, this is OK -- we could
             :     // have a case like TestRandomAccess.TestFuzz3, in which a single batch
             :     // DELETED from the DRS, INSERTed into MRS, and DELETED from MRS. In that case,
             :     // the timestamp of the last REDO will be the same and we can pick whichever
             :     // we like.
             :     const bool debug_ret = left_latest->timestamp() < right_latest->timestamp();
             :     CHECK_EQ(ret, debug_ret);
             :   }
             : #endif
             :   return ret;
             : }
same


http://gerrit.cloudera.org:8080/#/c/5096/3/src/kudu/tablet/compaction.h
File src/kudu/tablet/compaction.h:

PS3, Line 167: bool operator<(const CompactionInputRow& lhs, const CompactionInputRow& rhs);
this seems out of place in this patch (the commit message only mentions Timestamp) and actually goes against the reinsert stuff I have in-flight where I completely change the comparison between two CompactionInputRows, mind removing this from this patch?


-- 
To view, visit http://gerrit.cloudera.org:8080/5096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.

Change subject: [Timestamp] use 'operator<' instead of ComesBefore
......................................................................


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5096/3/src/kudu/common/timestamp.h
File src/kudu/common/timestamp.h:

Line 87: inline int Timestamp::CompareTo(const Timestamp& other) const {
Do we still need this routine or is this leftover ?


Line 109:   return rhs < lhs;
Should this be rhs.v < lhs.v ?


Line 113:   return !(lhs > rhs);
same as above.


Line 117:   return !(lhs < rhs);
same as above.


-- 
To view, visit http://gerrit.cloudera.org:8080/5096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5096

to look at the new patch set (#3).

Change subject: [Timestamp] use 'operator<' instead of ComesBefore
......................................................................

[Timestamp] use 'operator<' instead of ComesBefore

Added more syntactic sugar, now for the kudu::Timestamp class:
use operator{<, <=, >, <=, ==} instead of Timestamp::ComesBefore()
and Timestamp::CompareTo(), where possible.
Also, did a minor clean-up on the set of header files and forward
declarations.

This patch does not contain any functional changes.

Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
---
M src/kudu/common/timestamp.cc
M src/kudu/common/timestamp.h
M src/kudu/server/hybrid_clock.cc
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/delta_stats.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tserver/tablet_service.cc
10 files changed, 122 insertions(+), 99 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/5096/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
David Ribeiro Alves has posted comments on this change.

Change subject: [Timestamp] use 'operator<' instead of ComesBefore
......................................................................


Patch Set 5: Code-Review+2

lgtm, since the tidy bot is nt your fault I'm merging this.

-- 
To view, visit http://gerrit.cloudera.org:8080/5096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Dinesh Bhat, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5096

to look at the new patch set (#4).

Change subject: [Timestamp] use 'operator<' instead of ComesBefore
......................................................................

[Timestamp] use 'operator<' instead of ComesBefore

Added more syntactic sugar, now for the kudu::Timestamp class:
use operator{<, <=, >, <=, ==} instead of Timestamp::ComesBefore()
and Timestamp::CompareTo(), where possible.
Besides, this patch contains a minor clean-up on the of header files
and forward declarations.

This patch does not contain any functional changes.

Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
---
M src/kudu/common/timestamp.cc
M src/kudu/common/timestamp.h
M src/kudu/server/hybrid_clock.cc
M src/kudu/tablet/delta_stats.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tserver/tablet_service.cc
8 files changed, 74 insertions(+), 53 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/5096/4
-- 
To view, visit http://gerrit.cloudera.org:8080/5096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5096

to look at the new patch set (#2).

Change subject: [Timestamp] use 'operator<' instead of ComesBefore
......................................................................

[Timestamp] use 'operator<' instead of ComesBefore

Added more syntactic sugar, now for the kudu::Timestamp class:
use operator{<, <=, >, <=, ==} instead of Timestamp::ComesBefore()
and Timestamp::CompareTo(), where possible.
Also, did a minor clean-up on the set of header files and forward
declarations.

This patch does not contain any functional changes.

Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
---
M src/kudu/common/timestamp.cc
M src/kudu/common/timestamp.h
M src/kudu/server/hybrid_clock.cc
M src/kudu/tablet/compaction.h
M src/kudu/tablet/delta_stats.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tserver/tablet_service.cc
9 files changed, 75 insertions(+), 54 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/5096/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: [Timestamp] use 'operator<' instead of ComesBefore
......................................................................


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5096/3/src/kudu/common/timestamp.h
File src/kudu/common/timestamp.h:

Line 87: inline int Timestamp::CompareTo(const Timestamp& other) const {
> Do we still need this routine or is this leftover ?
The only usage of this now is in the template DeltaKey::CompareTo<>() method, which I found is not that easy to remove (yes, it would be possible to replace the call of this method with multiple comparisons down there, but I found it not elegant).  Speaking of properly modifying the DeltaKey::CompareTo<>-related stuff, I would do that in a separate changelist.

However, once that done we can obsolete this method.


Line 109:   return rhs < lhs;
> Should this be rhs.v < lhs.v ?
Nope, this is how you define '>' operator using '<': you just swap arguments :)


Line 113:   return !(lhs > rhs);
> same as above.
same as above, too, but now for '<=' and '>' operator pairs :)


Line 117:   return !(lhs < rhs);
> same as above.
same as above, too :)


-- 
To view, visit http://gerrit.cloudera.org:8080/5096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] [Timestamp] use 'operator<' instead of ComesBefore

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.

Change subject: [Timestamp] use 'operator<' instead of ComesBefore
......................................................................


Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5096/3/src/kudu/common/timestamp.h
File src/kudu/common/timestamp.h:

Line 109:   return rhs < lhs;
> Nope, this is how you define '>' operator using '<': you just swap argument
oh yeah, I missed that :), thanks.


-- 
To view, visit http://gerrit.cloudera.org:8080/5096
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I4a5b0f90e92f6de40822cfe872b417cf0c53121a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes