[kudu-CR] Add a ResultTracker class that will track server side results

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 26: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3190/26/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:

Line 244:   // CompletionRecords.
OK, I still think this iteration order thing is fragile, but I've made a note 
to come back to it.


http://gerrit.cloudera.org:8080/#/c/3190/26/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:

Line 131: // result_tracker_->TrackRpcOrChangeDriver(request_id);
woohoo, I like this much better. thanks for taking the time to re-work it


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 26
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add a ResultTracker class that will track server side results

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: Add a ResultTracker class that will track server side results
..


Add a ResultTracker class that will track server side results

This adds the initial version of the ResultTracker class that will be
responsible for tracking responses when we want exactly once call semantics.

While this is minimally working, i.e it's able to track responses and
can be used for exactly once semantics, it's not complete. Future patches
will address the missing functionality.
Still missing are:
- Time based client state cleaning.
- Watermark based per client state cleaning.

I initially had a unit test for this class, but it relied on templating
out the RpcContext (since it's not easy to build one for a unit test
and it's not clear what it would do) which became problematic as the
class grew. So I decided to add a new rpc-stress-tess that will test
this class within the rpc framework. I feel it's easier to do it that
way and that, since the client is not being used, the test can still
be very straight forward. This test is in a followup patch as it can
only run after the integration with the rest of the rpc subsystem.

Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Reviewed-on: http://gerrit.cloudera.org:8080/3190
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/result_tracker.cc
A src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc_context.h
4 files changed, 599 insertions(+), 0 deletions(-)

Approvals:
  Todd Lipcon: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 27
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a ResultTracker class that will track server side results

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 26:

Build Started http://104.196.14.100/job/kudu-gerrit/2338/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 26
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add a ResultTracker class that will track server side results
..

Add a ResultTracker class that will track server side results

This adds the initial version of the ResultTracker class that will be
responsible for tracking responses when we want exactly once call semantics.

While this is minimally working, i.e it's able to track responses and
can be used for exactly once semantics, it's not complete. Future patches
will address the missing functionality.
Still missing are:
- Time based client state cleaning.
- Watermark based per client state cleaning.

I initially had a unit test for this class, but it relied on templating
out the RpcContext (since it's not easy to build one for a unit test
and it's not clear what it would do) which became problematic as the
class grew. So I decided to add a new rpc-stress-tess that will test
this class within the rpc framework. I feel it's easier to do it that
way and that, since the client is not being used, the test can still
be very straight forward. This test is in a followup patch as it can
only run after the integration with the rest of the rpc subsystem.

Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/result_tracker.cc
A src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc_context.h
4 files changed, 599 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/3190/26
-- 
To view, visit http://gerrit.cloudera.org:8080/3190
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 26
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 25:

Todd: As we discussed I also changed the TrackRpc/ChangeDriver methods. I added 
a new TrackRpcUnlocked and a TrackRpcOrChangeDriver methods. The latter tracks 
the RPC or changes the driver, atomically.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 25
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 22:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3190/22/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:

Line 203:   // from before we reply to all the other ongoing ones.
> So its like this:
actually this is only important for the Fail* methods, but made sense to make 
iteration the same all over.


http://gerrit.cloudera.org:8080/#/c/3190/22/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:

PS22, Line 44: client ID and same sequence number
> typo: same client ID and sequence number
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 22
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add a ResultTracker class that will track server side results

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 22:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3190/22/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:

Line 203:   // from before we reply to all the other ongoing ones.
why are we guaranteed that 'completion_record' that we're copying from is at 
the beginning of the list? if I understand correctly, you're saying we go in 
reverse order so we get to 'completion_record' last, but don't understand why 
that property holds


http://gerrit.cloudera.org:8080/#/c/3190/22/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:

PS22, Line 44: client ID and same sequence number
typo: same client ID and sequence number


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 22
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add a ResultTracker class that will track server side results

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 24:

Build Started http://104.196.14.100/job/kudu-gerrit/2307/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 24
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add a ResultTracker class that will track server side results
..

Add a ResultTracker class that will track server side results

This adds the initial version of the ResultTracker class that will be
responsible for tracking responses when we want exactly once call semantics.

While this is minimally working, i.e it's able to track responses and
can be used for exactly once semantics, it's not complete. Future patches
will address the missing functionality.
Still missing are:
- Time based client state cleaning.
- Watermark based per client state cleaning.

I initially had a unit test for this class, but it relied on templating
out the RpcContext (since it's not easy to build one for a unit test
and it's not clear what it would do) which became problematic as the
class grew. So I decided to add a new rpc-stress-tess that will test
this class within the rpc framework. I feel it's easier to do it that
way and that, since the client is not being used, the test can still
be very straight forward. This test is in a followup patch as it can
only run after the integration with the rest of the rpc subsystem.

Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/result_tracker.cc
A src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc_context.h
4 files changed, 597 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/3190/24
-- 
To view, visit http://gerrit.cloudera.org:8080/3190
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 24
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a ResultTracker class that will track server side results

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 23:

Build Started http://104.196.14.100/job/kudu-gerrit/2306/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 23
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 22: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 22
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 22:

unrelated failure of LinkedListTest.TestLoadAndVerify

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 22
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add a ResultTracker class that will track server side results
..

Add a ResultTracker class that will track server side results

This adds the initial version of the ResultTracker class that will be
responsible for tracking responses when we want exactly once call semantics.

While this is minimally working, i.e it's able to track responses and
can be used for exactly once semantics, it's not complete. Future patches
will address the missing functionality.
Still missing are:
- Time based client state cleaning.
- Watermark based per client state cleaning.

I initially had a unit test for this class, but it relied on templating
out the RpcContext (since it's not easy to build one for a unit test
and it's not clear what it would do) which became problematic as the
class grew. So I decided to add a new rpc-stress-tess that will test
this class within the rpc framework. I feel it's easier to do it that
way and that, since the client is not being used, the test can still
be very straight forward. This test is in a followup patch as it can
only run after the integration with the rest of the rpc subsystem.

Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/result_tracker.cc
A src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc_context.h
4 files changed, 597 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/3190/22
-- 
To view, visit http://gerrit.cloudera.org:8080/3190
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 22
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add a ResultTracker class that will track server side results
..

Add a ResultTracker class that will track server side results

This adds the initial version of the ResultTracker class that will be
responsible for tracking responses when we want exactly once call semantics.

While this is minimally working, i.e it's able to track responses and
can be used for exactly once semantics, it's not complete. Future patches
will address the missing functionality.
Still missing are:
- Time based client state cleaning.
- Watermark based per client state cleaning.

I initially had a unit test for this class, but it relied on templating
out the RpcContext (since it's not easy to build one for a unit test
and it's not clear what it would do) which became problematic as the
class grew. So I decided to add a new rpc-stress-tess that will test
this class within the rpc framework. I feel it's easier to do it that
way and that, since the client is not being used, the test can still
be very straight forward. This test is in a followup patch as it can
only run after the integration with the rest of the rpc subsystem.

Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/result_tracker.cc
A src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc_context.h
4 files changed, 591 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/3190/21
-- 
To view, visit http://gerrit.cloudera.org:8080/3190
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 21
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a ResultTracker class that will track server side results

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 21:

Build Started http://104.196.14.100/job/kudu-gerrit/2257/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 21
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a ResultTracker class that will track server side results

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 20:

Build Started http://104.196.14.100/job/kudu-gerrit/2248/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 20
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add a ResultTracker class that will track server side results
..

Add a ResultTracker class that will track server side results

This adds the initial version of the ResultTracker class that will be
responsible for tracking responses when we want exactly once call semantics.

While this is minimally working, i.e it's able to track responses and
can be used for exactly once semantics, it's not complete. Future patches
will address the missing functionality.
Still missing are:
- Time based client state cleaning.
- Watermark based per client state cleaning.

I initially had a unit test for this class, but it relied on templating
out the RpcContext (since it's not easy to build one for a unit test
and it's not clear what it would do) which became problematic as the
class grew. So I decided to add a new rpc-stress-tess that will test
this class within the rpc framework. I feel it's easier to do it that
way and that, since the client is not being used, the test can still
be very straight forward. This test is in a followup patch as it can
only run after the integration with the rest of the rpc subsystem.

Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/result_tracker.cc
A src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc_context.h
4 files changed, 591 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/3190/20
-- 
To view, visit http://gerrit.cloudera.org:8080/3190
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 20
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a ResultTracker class that will track server side results

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 18:

Build Started http://104.196.14.100/job/kudu-gerrit/2236/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a ResultTracker class that will track server side results

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 17:

(29 comments)

http://gerrit.cloudera.org:8080/#/c/3190/17/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:

Line 54:   }
what do you think about adding a utility function to map-util like 
https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html#computeIfAbsent-K-java.util.function.Function-
 ?

Then the above 10 lines or so could be:

  ClientState* client_state = ComputeIfAbsent(_, request_id.client_id(),
   []{ return unique_ptr(new ClientState()); }).get();


Line 76: CHECK(client_state->completion_records.emplace(request_id.seq_no(),
I think you can just emplace(request_id.seq_no(), completion_record) here 
without constructing a unique_ptr and std::moving it, since unique_ptr has a 
unique_ptr(T* val) constructor

(same is true above)


PS17, Line 85: means a tablet is being bootstrapped for the second time,
since this code is in the RPC system I think it would be better to be more 
general in the description here. Plus I think you decided that this isn't the 
only case in which this happens, right?


Line 123:   CompletionRecord* completion_record = cr_it->second.get();
another potential for a map-util function here


Line 153:   // ... if we did find a CompletionRecord return true if we're the 
driver of false
s/of/or/


Line 158: void ResultTracker::LogAndTraceAndRespondSuccess(RpcContext* context,
these methods are duplicating a lot of RpcContext. Remind me again why we can't 
just call context->RespondSuccess()? (perhaps after breaking it out into one 
that takes a PB instead of using 'response_pb_'


PS17, Line 191: e(
nit: add 'Unlocked' to the name


PS17, Line 194: PREDICT_TRUE
no need for PREDICT inside CHECK (it's already implicitly part of CHECK)


Line 206: const Message* response) {
should this be a reference? it doesn't look like it hangs onto the pointer


Line 219:   CHECK(completion_record->driver_attempt_no == 
request_id.attempt_no());
CHECK_EQ


Line 235:   completion_record->ongoing_rpcs.erase(orpc_iter.base()));
hrm, not really familiar with what this is doing. it seems odd though that you 
increment it and _then_ erase it...


http://gerrit.cloudera.org:8080/#/c/3190/17/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:

Line 19: #include 
can you get away with a forward decl? am guessing to do so you'll have to 
define ctor and ctor in the .cc file instead of implicit ones, but that's 
probably a good idea anyway


Line 28: #include "kudu/gutil/stl_util.h"
nit: sort


Line 35: // A ResultTracker for RPC results.
somewhere in here I think it would be worth a sentence or two saying that in 
most cases this is internal to the RPC system (ie handlers don't call the 
Respond* methods directly)


PS17, Line 38: sequence number
sequence number and client ID


PS17, Line 41: rpc
nit: s/rpc/RPC/ here and elsewhere


PS17, Line 44: id 
nit: capitalize ID here and elsewhere


PS17, Line 61: being completed,
being handled


PS17, Line 70: od 
methods


PS17, Line 81: be the only handler,
this isn't necessarily true -- the client could have gotten a timeout and 
decided to retry while the original attempt was still running, right?


PS17, Line 84: hanlder 
typo


Line 85: // previous leader originating update.
I think this description is missing one key thing that distinguishes this case 
from the "two retries on the same server" case. Here, we actually know a priori 
that the original leader-originated operation is the one that is going to 
succeed (it has to, because it has been committed), and therefore it needs to 
"steal" the handler role, even if it arrives after a client-originated request. 
However, in the case that you have a non-replicated operation (like two 
client-originated requests) you are still free to arbitrarily assign which one 
gets the ownership and let the other one tack on.

So this whole section is really about "stealing ownership" rather than 
"multiple handlers", right?


PS17, Line 89:  must be handled 
not sure what this means.. do you mean that the responses must be mutated 
exclusively by one handler?


PS17, Line 94: there o
missing a word


Line 182:   void RecordCompletionAndRespond(const RequestIdPB& request_id,
This and the three methods below are only called via RpcContext, right? I think 
it's worth noting for each method whether it's "user-facing" or "rpc-system 
internal"


Line 221: int64_t driver_attempt_no;
what about if it's completed? this is the attempt number that successfully 
handled it?


Line 227:   struct ClientState {
doc


PS17, Line 242: it's
nit: its

also, I dont know what "its own" means, exactly


Line 243:   // 2 - It's the driver of the RPC and the RPC has no handler (was 
attached).
what is "was attached"?


-- 
To view, visit 

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add a ResultTracker class that will track server side results
..

Add a ResultTracker class that will track server side results

This adds the initial version of the ResultTracker class that will be
responsible for tracking responses when we want exactly once call semantics.

While this is minimally working, i.e it's able to track responses and
can be used for exactly once semantics, it's not complete. Future patches
will address the missing functionality.
Still missing are:
- Time based client state cleaning.
- Watermark based per client state cleaning.

I initially had a unit test for this class, but it relied on templating
out the RpcContext (since it's not easy to build one for a unit test
and it's not clear what it would do) which became problematic as the
class grew. So I decided to add a new rpc-stress-tess that will test
this class within the rpc framework. I feel it's easier to do it that
way and that, since the client is not being used, the test can still
be very straight forward. This test is in a followup patch as it can
only run after the integration with the rest of the rpc subsystem.

Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/result_tracker.cc
A src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc_context.h
4 files changed, 592 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/3190/17
-- 
To view, visit http://gerrit.cloudera.org:8080/3190
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a ResultTracker class that will track server side results

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 17:

Build Started http://104.196.14.100/job/kudu-gerrit/2162/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a ResultTracker class that will track server side results

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 16:

Build Started http://104.196.14.100/job/kudu-gerrit/2153/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add a ResultTracker class that will track server side results
..

Add a ResultTracker class that will track server side results

This adds the initial version of the ResultTracker class that will be
responsible for tracking responses when we want exactly once call semantics.

While this is minimally working, i.e it's able to track responses and
can be used for exactly once semantics, it's not complete. Future patches
will address the missing functionality.
Still missing are:
- Time based client state cleaning.
- Watermark based per client state cleaning.

I initially had a unit test for this class, but it relied on templating
out the RpcContext (since it's not easy to build one for a unit test
and it's not clear what it would do) which became problematic as the
class grew. So I decided to add a new rpc-stress-tess that will test
this class within the rpc framework. I feel it's easier to do it that
way and that, since the client is not being used, the test can still
be very straight forward. This test is in a followup patch as it can
only run after the integration with the rest of the rpc subsystem.

Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/result_tracker.cc
A src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc_context.h
4 files changed, 596 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/3190/16
-- 
To view, visit http://gerrit.cloudera.org:8080/3190
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a ResultTracker class that will track server side results

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 15:

Build Started http://104.196.14.100/job/kudu-gerrit/2134/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has uploaded a new patch set (#14).

Change subject: Add a ResultTracker class that will track server side results
..

Add a ResultTracker class that will track server side results

This adds the initial version of the ResultTracker class that will be
responsible for tracking responses when we want exactly once call semantics.

While this is minimally working, i.e it's able to track responses and
can be used for exactly once semantics, it's not complete. Future patches
will address the missing functionality.
Still missing are:
- Time based client state cleaning.
- Watermark based per client state cleaning.

I initially had a unit test for this class, but it relied on templating
out the RpcContext (since it's not easy to build one for a unit test
and it's not clear what it would do) which became problematic as the
class grew. So I decided to add a new rpc-stress-tess that will test
this class within the rpc framework. I feel it's easier to do it that
way and that, since the client is not being used, the test can still
be very straight forward. This test is in a followup patch as it can
only run after the integration with the rest of the rpc subsystem.

Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/result_tracker.cc
A src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc_context.h
4 files changed, 497 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/3190/14
-- 
To view, visit http://gerrit.cloudera.org:8080/3190
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 13: Verified+1

unrelated flake ReplicatedAlterTableTest.TestReplicatedAlter

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3190/12/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:

Line 221: }
> lots of dup code in the above methods. any refactor doable? (even one that 
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add a ResultTracker class that will track server side results

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 12:

(25 comments)

http://gerrit.cloudera.org:8080/#/c/3190/12/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:

Line 53:   if (completion_record == nullptr) {
PREDICT_TRUE would be nice documentation value here that this is the hot path


Line 71:   DCHECK_NOTNULL(response)->CopyFrom(*completion_record->response);
this block might be somewhat expensive to do inside the lock... I guess it's OK 
because we assume this is a rare code path?


Line 72:   DCHECK_NOTNULL(context)->call_->RespondSuccess(*response);
reaching into the guts of RpcContext seems a little odd here.


PS12, Line 146: "
nit: missing space


Line 176:   CHECK(completion_record->response == NULL) << "This request was 
previously marked as successful.";
dumping the response stringified here would be useful for debugging such a crash


Line 221: }
lots of dup code in the above methods. any refactor doable? (even one that 
takes a lambda of what to do with each ongoing_rpc)?


http://gerrit.cloudera.org:8080/#/c/3190/12/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:

Line 41: // When a call first arrives from the client the RPC subsystem will 
call CheckDuplicate() which
is this class entirely internal to the RPC system or would it be used by 
service implementors as well? I think some of my questions below might be moot 
based on your answer to this question.


PS12, Line 90: returns the status
I think this one-line summary is a bit inaccurate, since it actually will 
_handle_ the RPC in the case that it's a duplicate. Also, the name is 
inaccurate in the same way (see below for a suggestion)

Also, this registers the RPC as currently running, right? maybe 'StartRPC' is a 
better name? Should document whether there is any 'cleanup' that the caller 
needs to do here to avoid leaking it as an in-progress RPC.


Line 98:   RpcState CheckDuplicate(const RequestIdPB& request_id,
What if you broke out the guts of this method, so this just returned the 
RpcState, and you had a new method 'HandleIfDuplicate' which does what this 
method is doing?

Then you should be able to unit test this without mocking RpcContext?


PS12, Line 107: at 
nit: of


Line 108:   // RPC:
clarify: on the same server, right?


PS12, Line 112: FailAndRespond()
why does RPC2 call FailAndRespond here? not following


Line 114:   // 4 - RPC2 - Calls CheckDuplicate(), gets RpcState::NEW back.
still confused: I thought we just called FailAndRespond, why do we now call 
CheckDuplicate again?


Line 124:   // sure their attempt number is still the driver of the RPC and 
give up if it isn't.
I remember discussing a scenario like this before I went on vacation, but I 
can't quite remember the details, and the above explanation isn't quite clear 
to me. Perhaps a more concrete example (and some more clarity on the threads 
involved) would be useful?


PS12, Line 128: with from 
typo


PS12, Line 128: , 
nit: no comma


PS12, Line 129: attempts at this RPC 
retries of the same RPC?


Line 133:   void RecordCompletionAndRespond(const RequestIdPB& request_id,
I wonder whether this could be "hooked" in the RpcContext in such a way that it 
would happen automatically? ie once you associate an RpcContext with a 
retriable RPC, _any_ response to the RPC would need to record completion to 
avoid a leak and potential dangling reference, no? I'm worried that relying on 
the handlers to use this special 'record completion' might be tricky? (or is 
this called from RpcContext itself?)


PS12, Line 167: unique identifier 
where does this identifier come from? not sure what it corresponds to.


PS12, Line 167: driver
this 'driver' term shows up in this class a bit but seems new. is it in the 
design doc somewhere and I missed it?


Line 176: ~ClientState() { STLDeleteValues(_records); }
can you make it a map of unique_ptr<> now that we have c++11? then you don't 
need a ctor (and this struct would be safely moveable, whereas right now there 
would be a bug if you copied ClientState)


PS12, Line 181: DeleteC
Remove is probably better than Delete since it actually gets returned


PS12, Line 185: LogAndTraceFailure(
naming is inconsistent with the pattern above


PS12, Line 193: ClientState*>
unique_ptr?


Line 194: };
DISALLOW_COPY_AND_ASSIGN


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a ResultTracker class that will track server side results

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 10:

Build Started http://104.196.14.100/job/kudu-gerrit/1899/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add a ResultTracker class that will track server side results
..

Add a ResultTracker class that will track server side results

This adds the initial version of the ResultTracker class that will be
responsible for tracking responses when we want exactly once call semantics.

While this is minimally working, i.e it's able to track responses and
can be used for exactly once semantics, it's not complete. Future patches
will address the missing functionality.
Still missing are:
- Time based client state cleaning.
- Watermark based per client state cleaning.

I initially had a unit test for this class, but it relied on templating
out the RpcContext (since it's not easy to build one for a unit test
and it's not clear what it would do) which became problematic as the
class grew. So I decided to add a new rpc-stress-tess that will test
this class within the rpc framework. I feel it's easier to do it that
way and that, since the client is not being used, the test can still
be very straight forward. This test is in a followup patch as it can
only run after the integration with the rest of the rpc subsystem.

Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/result_tracker.cc
A src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc_context.h
4 files changed, 423 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/3190/10
-- 
To view, visit http://gerrit.cloudera.org:8080/3190
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add a ResultTracker class that will track server side results
..

Add a ResultTracker class that will track server side results

This adds the initial version of the ResultTracker class that will be
responsible for tracking responses when we want exactly once call semantics.

While this is minimally working, i.e it's able to track responses and
can be used for exactly once semantics, it's not complete. Future patches
will address the missing functionality.
Still missing are:
- Time based client state cleaning.
- Watermark based per client state cleaning.

I initially had a unit test for this class, but it relied on templating
out the RpcContext (since it's not easy to build one for a unit test
and it's not clear what it would do) which became problematic as the
class grew. So I decided to add a new rpc-stress-tess that will test
this class within the rpc framework. I feel it's easier to do it that
way and that, since the client is not being used, the test can still
be very straight forward. This test is in a followup patch as it can
only run after the integration with the rest of the rpc subsystem.

Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/result_tracker.cc
A src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc_context.h
4 files changed, 411 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/3190/9
-- 
To view, visit http://gerrit.cloudera.org:8080/3190
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a ResultTracker class that will track server side results

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 9:

Build Started http://104.196.14.100/job/kudu-gerrit/1884/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add a ResultTracker class that will track server side results
..

Add a ResultTracker class that will track server side results

This adds the initial version of the ResultTracker class that will be
responsible for tracking responses when we want exactly once call semantics.

While this is minimally working, i.e it's able to track responses and
can be used for exactly once semantics, it's not complete. Future patches
will address the missing functionality.
Still missing are:
- Time based client state cleaning.
- Watermark based per client state cleaning.

I initially had a unit test for this class, but it relied on templating
out the RpcContext (since it's not easy to build one for a unit test
and it's not clear what it would do) which became problematic as the
class grew. So I decided to add a new rpc-stress-tess that will test
this class within the rpc framework. I feel it's easier to do it that
way and that, since the client is not being used, the test can still
be very straight forward. This test is in a followup patch as it can
only run after the integration with the rest of the rpc subsystem.

Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/result_tracker.cc
A src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc_context.h
4 files changed, 402 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/3190/8
-- 
To view, visit http://gerrit.cloudera.org:8080/3190
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a ResultTracker class that will track server side results

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 8:

Build Started http://104.196.14.100/job/kudu-gerrit/1881/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a ResultTracker class that will track server side results

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3190/3/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:

Line 40:   lock_guard l(_);
> no, the lock protects access to clients_ and to the ClientState that belong
I suspect it will be, as nearly every RPC will call CheckDuplicate(), no?


Line 78:   LOG(FATAL) << "Wrong state";
> the whole point is that we only call this once per RPC, if the caller got N
OK, could we at least log what the wrong state was?


Line 119:   ClientState* client_state = FindOrDie(clients_, client_id);
:   CompletionRecord* completion_record = 
FindOrDie(client_state->completion_records,
:   sequence_number);
> why? if we are responding to an RPC that is not being tracked then it's an 
Well, this goes back to the lack of documentation thing. :)

You're designing a module with "hard" boundaries. That is, if one of us misuses 
the ResultTracker, it crashes.

Without context and in isolation, I tend to assume new modules should have 
"soft" boundaries, where misuse leads to returned errors. And to be fair, there 
are advantages to soft boundaries: it's easier to test such a module in 
isolation.

Anyway, there's nothing inherently wrong with hard boundaries, you just need to 
be very clear in the interface regarding what happens in the event of misuse.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add a ResultTracker class that will track server side results

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Add a ResultTracker class that will track server side results
..


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3190/6/src/kudu/rpc/result_tracker.h
File src/kudu/rpc/result_tracker.h:

Line 45: // of the RPC at that the corresponding server function should be 
executed. If any other value is
Nit: "at that the" --> "and that the"


Line 49: // client appropriately,
Nit: trailing comma here?


Line 55: // If, on the other hand, if execution of the server function is not 
successful then one of
Nit: too many 'ifs' here.


Line 96:   RpcState CheckDuplicate(const std::string& client_id,
I'd rename the function slightly to imply that it's not just a stateless 
"check". That is, it'll actually register the caller's input internally such 
that RecordCompletionAndRespond() does something with it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add a ResultTracker class that will track server side results
..

Add a ResultTracker class that will track server side results

This adds the initial version of the ResultTracker class that will be
responsible for tracking responses when we want exactly once call semantics.

While this is minimally working, i.e it's able to track responses and
can be used for exactly once semantics, it's not complete. Future patches
will addressing the missing functionality.
Still missing are:
- Time based client state cleaning.
- Watermark based per client state cleaning.

I initially had a unit test for this class, but it relied on templating
out the RpcContext (since it's not easy to build one for a unit test
and it's not clear waht it would do) which became problematic as the
class grew. So I decided to add a new rpc-stress-tess that will test
this class within the rpc framework. I feel it's easier to do it that
way and that, since the client is not being used, the test can still
be very straight forward. This test is in a followup patch as it can
only run after the integration with the rest of the rpc subsystem.

Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/result_tracker.cc
A src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc_context.h
4 files changed, 341 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a ResultTracker class that will track server side results

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has uploaded a new change for review.

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

Change subject: Add a ResultTracker class that will track server side results
..

Add a ResultTracker class that will track server side results

This adds the initial version of the ResultTracker class that will be
responsible for tracking responses when we want exactly once call semantics.

While this is minimally working, i.e it's able to track responses and
can be used for exactly once semantics, it's not complete. Future patches
will addressing the missing functionality.
Still missing are:
- Time based client state cleaning.
- Watermark based per client state cleaning.

I initially had a unit test for this class, but it relied on templating
out the RpcContext (since it's not easy to build one for a unit test
and it's not clear waht it would do) which became problematic as the
class grew. So I decided to add a new rpc-stress-tess that will test
this class within the rpc framework. I feel it's easier to do it that
way and that, since the client is not being used, the test can still
be very straight forward. This test is in a followup patch as it can
only run after the integration with the rest of the rpc subsystem.

Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
---
A src/kudu/rpc/result_tracker.cc
A src/kudu/rpc/result_tracker.h
2 files changed, 347 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6718951a9998a6c9b0db35e8f09ff8304591e8b1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves