You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Andrew Wong (Code Review)" <ge...@cloudera.org> on 2020/02/29 03:35:04 UTC

[kudu-CR] util: add a deadline to BlockingQueue::BlockingPut() 

Hello Adar Dembo,

I'd like you to do a code review. Please visit

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

to review the following change.


Change subject: util: add a deadline to BlockingQueue::BlockingPut()
......................................................................

util: add a deadline to BlockingQueue::BlockingPut()

Drawing inspiration from BlockingDrainTo(), this adds a deadline to
BlockingPut(), with the same error semantics.

This also updates SubprocessServer to use the the new method, instead of
manual deadline-checking.

Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
---
M src/kudu/consensus/log.cc
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/subprocess_server-test.cc
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
5 files changed, 50 insertions(+), 39 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
Gerrit-Change-Number: 15324
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>

[kudu-CR] util: deadline for BlockingQueue::Blocking{Put,Get}

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15324 )

Change subject: util: deadline for BlockingQueue::Blocking{Put,Get}
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15324/5/src/kudu/subprocess/server.cc
File src/kudu/subprocess/server.cc:

http://gerrit.cloudera.org:8080/#/c/15324/5/src/kudu/subprocess/server.cc@293
PS5, Line 293: } // namespace subprocess
> Didn't remove the old function site.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
Gerrit-Change-Number: 15324
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 03 Mar 2020 06:20:53 +0000
Gerrit-HasComments: Yes

[kudu-CR] util: deadline for BlockingQueue::Blocking{Put,Get}

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: util: deadline for BlockingQueue::Blocking{Put,Get}
......................................................................

util: deadline for BlockingQueue::Blocking{Put,Get}

Drawing inspiration from BlockingDrainTo(), this adds a deadline to
BlockingPut() and BlockingGet(), with the same error semantics.

This also updates SubprocessServer to use of BlockingPut(), instead of
manual deadline-checking.

Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
---
M src/kudu/consensus/log.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/subprocess_server-test.cc
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
6 files changed, 141 insertions(+), 70 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
Gerrit-Change-Number: 15324
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] util: deadline for BlockingQueue::Blocking{Put,Get}

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: util: deadline for BlockingQueue::Blocking{Put,Get}
......................................................................

util: deadline for BlockingQueue::Blocking{Put,Get}

Drawing inspiration from BlockingDrainTo(), this adds a deadline to
BlockingPut() and BlockingGet(), with the same error semantics.

This also updates SubprocessServer to use of BlockingPut(), instead of
manual deadline-checking.

Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
---
M src/kudu/consensus/log.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/subprocess/subprocess_server-test.cc
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
7 files changed, 152 insertions(+), 77 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
Gerrit-Change-Number: 15324
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] util: deadline for BlockingQueue::Blocking{Put,Get}

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: util: deadline for BlockingQueue::Blocking{Put,Get}
......................................................................

util: deadline for BlockingQueue::Blocking{Put,Get}

Drawing inspiration from BlockingDrainTo(), this adds a deadline to
BlockingPut() and BlockingGet(), with the same error semantics.

This also updates SubprocessServer to use of BlockingPut(), instead of
manual deadline-checking.

Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
---
M src/kudu/consensus/log.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/subprocess_server-test.cc
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
6 files changed, 138 insertions(+), 68 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
Gerrit-Change-Number: 15324
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] util: deadline for BlockingQueue::Blocking{Put,Get}

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15324 )

Change subject: util: deadline for BlockingQueue::Blocking{Put,Get}
......................................................................


Patch Set 6: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
Gerrit-Change-Number: 15324
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 03 Mar 2020 06:21:41 +0000
Gerrit-HasComments: No

[kudu-CR] util: deadline for BlockingQueue::Blocking{Put,Get}

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: util: deadline for BlockingQueue::Blocking{Put,Get}
......................................................................

util: deadline for BlockingQueue::Blocking{Put,Get}

Drawing inspiration from BlockingDrainTo(), this adds a deadline to
BlockingPut() and BlockingGet(), with the same error semantics.

This also updates SubprocessServer to use of BlockingPut(), instead of
manual deadline-checking.

Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
---
M src/kudu/consensus/log.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/subprocess/subprocess_server-test.cc
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
7 files changed, 154 insertions(+), 73 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
Gerrit-Change-Number: 15324
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] util: deadline for BlockingQueue::Blocking{Put,Get}

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15324 )

Change subject: util: deadline for BlockingQueue::Blocking{Put,Get}
......................................................................


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/15324/5/src/kudu/subprocess/server.cc
File src/kudu/subprocess/server.cc:

http://gerrit.cloudera.org:8080/#/c/15324/5/src/kudu/subprocess/server.cc@293
PS5, Line 293: Status SubprocessServer::QueueCall(const shared_ptr<SubprocessCall>& call) {
Didn't remove the old function site.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
Gerrit-Change-Number: 15324
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 03 Mar 2020 05:47:48 +0000
Gerrit-HasComments: Yes

[kudu-CR] util: deadline for BlockingQueue::Blocking{Put,Get}

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: util: deadline for BlockingQueue::Blocking{Put,Get}
......................................................................

util: deadline for BlockingQueue::Blocking{Put,Get}

Drawing inspiration from BlockingDrainTo(), this adds a deadline to
BlockingPut() and BlockingGet(), with the same error semantics.

This also updates SubprocessServer to use of BlockingPut(), instead of
manual deadline-checking.

Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
---
M src/kudu/consensus/log.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/subprocess_server-test.cc
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
6 files changed, 138 insertions(+), 68 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
Gerrit-Change-Number: 15324
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] util: deadline for BlockingQueue::Blocking{Put,Get}

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15324 )

Change subject: util: deadline for BlockingQueue::Blocking{Put,Get}
......................................................................


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/15324/4/src/kudu/subprocess/server.cc
File src/kudu/subprocess/server.cc:

http://gerrit.cloudera.org:8080/#/c/15324/4/src/kudu/subprocess/server.cc@292
PS4, Line 292: Status SubprocessServer::QueueCall(const shared_ptr<SubprocessCall>& call) {
It's only called in one place; maybe just inline it?


http://gerrit.cloudera.org:8080/#/c/15324/4/src/kudu/util/blocking_queue-test.cc
File src/kudu/util/blocking_queue-test.cc:

http://gerrit.cloudera.org:8080/#/c/15324/4/src/kudu/util/blocking_queue-test.cc@108
PS4, Line 108:   ASSERT_OK(test_queue.BlockingPut(3, MonoTime::Now() + kShortTimeout));
If this fires, join() is never called. Should use SCOPED_CLEANUP to avoid.

Below too.


http://gerrit.cloudera.org:8080/#/c/15324/4/src/kudu/util/blocking_queue.h
File src/kudu/util/blocking_queue.h:

http://gerrit.cloudera.org:8080/#/c/15324/4/src/kudu/util/blocking_queue.h@187
PS4, Line 187:     if (deadline.Initialized() && MonoTime::Now() > deadline) {
             :       return Status::TimedOut("");
             :     }
The other operations don't check the timeout up front. Should they? Why the discrepancy in semantics?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
Gerrit-Change-Number: 15324
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Mon, 02 Mar 2020 23:10:44 +0000
Gerrit-HasComments: Yes

[kudu-CR] util: deadline for BlockingQueue::Blocking{Put,Get}

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15324 )

Change subject: util: deadline for BlockingQueue::Blocking{Put,Get}
......................................................................

util: deadline for BlockingQueue::Blocking{Put,Get}

Drawing inspiration from BlockingDrainTo(), this adds a deadline to
BlockingPut() and BlockingGet(), with the same error semantics.

This also updates SubprocessServer to use of BlockingPut(), instead of
manual deadline-checking.

Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
Reviewed-on: http://gerrit.cloudera.org:8080/15324
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/consensus/log.cc
M src/kudu/integration-tests/linked_list-test-util.h
M src/kudu/subprocess/server.cc
M src/kudu/subprocess/server.h
M src/kudu/subprocess/subprocess_server-test.cc
M src/kudu/util/blocking_queue-test.cc
M src/kudu/util/blocking_queue.h
7 files changed, 152 insertions(+), 77 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
Gerrit-Change-Number: 15324
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] util: deadline for BlockingQueue::Blocking{Put,Get}

Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15324 )

Change subject: util: deadline for BlockingQueue::Blocking{Put,Get}
......................................................................


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/15324/4/src/kudu/subprocess/server.cc
File src/kudu/subprocess/server.cc:

http://gerrit.cloudera.org:8080/#/c/15324/4/src/kudu/subprocess/server.cc@292
PS4, Line 292: 
> It's only called in one place; maybe just inline it?
Done


http://gerrit.cloudera.org:8080/#/c/15324/4/src/kudu/util/blocking_queue-test.cc
File src/kudu/util/blocking_queue-test.cc:

http://gerrit.cloudera.org:8080/#/c/15324/4/src/kudu/util/blocking_queue-test.cc@108
PS4, Line 108:   });
> If this fires, join() is never called. Should use SCOPED_CLEANUP to avoid.
Done


http://gerrit.cloudera.org:8080/#/c/15324/4/src/kudu/util/blocking_queue.h
File src/kudu/util/blocking_queue.h:

http://gerrit.cloudera.org:8080/#/c/15324/4/src/kudu/util/blocking_queue.h@187
PS4, Line 187:   // - OK if successful
             :   // - TimedOut if the deadline passed
             :   // 
> The other operations don't check the timeout up front. Should they? Why the
I copied the exiting behavior of BlockingDrainTo() w.r.t. BlockingGet(), by which elements are returned as long as there is stuff in the queue, which seems reasonable and important (e.g. callers may need to process all elements that are in the queue regardless of deadline). That seems less important for BlockingPut().

I left a comment here.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If32bad5667195d72eeb03e4942751824d4dadb7a
Gerrit-Change-Number: 15324
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 03 Mar 2020 05:44:23 +0000
Gerrit-HasComments: Yes