[kudu-CR] KUDU-2038: Support bitmap index

2018-11-19 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11722 )

Change subject: KUDU-2038: Support bitmap index
..


Patch Set 6:

> Here's a design doc that LiFu shared with me via Slack. It was originally a 
> docx, but I've uploaded it as a gdoc to facilitate comments. If there's 
> trouble accessing it, I can funnel comments from the gdoc to gerrit.
>
> https://docs.google.com/document/d/1U_RFs8piw30K7fjyl0m08v2f-eiIjOUOkJRGGXrCoVY/edit?usp=sharing

Thanks, Andrew.

Lifu, I've left some comments on this gdoc. Could we continue this design 
discussion there? Let us know if you're not able to and we'll find a different 
approach.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0edaa0ef1dba2dbce85ebf15f0a731e4939a7860
Gerrit-Change-Number: 11722
Gerrit-PatchSet: 6
Gerrit-Owner: helifu 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: helifu 
Gerrit-Comment-Date: Mon, 19 Nov 2018 17:51:46 +
Gerrit-HasComments: No


[kudu-CR] [tests] fix flake in TestRandomHistoryGCWorkload

2018-11-16 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11945 )

Change subject: [tests] fix flake in TestRandomHistoryGCWorkload
..


Patch Set 1: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11945/1/src/kudu/integration-tests/tablet_history_gc-itest.cc
File src/kudu/integration-tests/tablet_history_gc-itest.cc:

http://gerrit.cloudera.org:8080/#/c/11945/1/src/kudu/integration-tests/tablet_history_gc-itest.cc@519
PS1, Line 519: # if !defined(THREAD_SANITIZER)
 :   OverrideFlagForSlowTests("test_num_rounds",
 :Substitute("$0", 
FLAGS_test_num_rounds * 5));
 : # endif
So before this change, in TSAN mode, the test would time out due to the high 
number of rounds?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d146d4b83c8d488d3c1766a53fe4a4d322b6590
Gerrit-Change-Number: 11945
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 16 Nov 2018 21:13:26 +
Gerrit-HasComments: Yes


[kudu-CR] Fix some issues in HMS and Sentry test fixtures

2018-11-16 Thread Adar Dembo (Code Review)
Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/11943 )

Change subject: Fix some issues in HMS and Sentry test fixtures
..

Fix some issues in HMS and Sentry test fixtures

1. The HMS or Sentry client object may be null in TearDown. If we don't
   check, we'll deref a null pointer if there's an error during SetUp()
   (e.g. if the HMS or Sentry server failed to start).
2. The HMSCatalogTest fixture should properly chain to its superclass.

Change-Id: Ib0376b972fe6add6d9312aea6944c9ab1a03f25f
Reviewed-on: http://gerrit.cloudera.org:8080/11943
Reviewed-by: Hao Hao 
Tested-by: Kudu Jenkins
---
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/sentry/sentry-test-base.h
2 files changed, 8 insertions(+), 2 deletions(-)

Approvals:
  Hao Hao: Looks good to me, approved
  Kudu Jenkins: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib0376b972fe6add6d9312aea6944c9ab1a03f25f
Gerrit-Change-Number: 11943
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)


[kudu-CR] Fix some issues in HMS and Sentry test fixtures

2018-11-16 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11943 )

Change subject: Fix some issues in HMS and Sentry test fixtures
..


Patch Set 1:

Here's problem #1 in the wild: 
http://dist-test.cloudera.org:8080/diagnose?key=bad20370-e4da-11e8-9b35-0242ac110002


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib0376b972fe6add6d9312aea6944c9ab1a03f25f
Gerrit-Change-Number: 11943
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 16 Nov 2018 18:07:57 +
Gerrit-HasComments: No


[kudu-CR] Fix some issues in HMS and Sentry test fixtures

2018-11-16 Thread Adar Dembo (Code Review)
Hello Andrew Wong, Hao Hao,

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

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

to review the following change.


Change subject: Fix some issues in HMS and Sentry test fixtures
..

Fix some issues in HMS and Sentry test fixtures

1. The HMS or Sentry client object may be null in TearDown. If we don't
   check, we'll deref a null pointer if there's an error during SetUp()
   (e.g. if the HMS or Sentry server failed to start).
2. The HMSCatalogTest fixture should properly chain to its superclass.

Change-Id: Ib0376b972fe6add6d9312aea6944c9ab1a03f25f
---
M src/kudu/hms/hms_catalog-test.cc
M src/kudu/sentry/sentry-test-base.h
2 files changed, 8 insertions(+), 2 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib0376b972fe6add6d9312aea6944c9ab1a03f25f
Gerrit-Change-Number: 11943
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 


[kudu-CR] raft consensus nonvoter-itest: deflake a bit

2018-11-14 Thread Adar Dembo (Code Review)
Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/11918 )

Change subject: raft_consensus_nonvoter-itest: deflake a bit
..

raft_consensus_nonvoter-itest: deflake a bit

I saw a failure in ReplicaBehindWalGcThresholdITest.ReplicaReplacement
(GetParam() was (1, false)) just after the master was restarted:

  raft_consensus_nonvoter-itest.cc:2070: Failure
  Failed
  Bad status: Service unavailable: Leader not yet ready to serve requests

This is odd as there's a WaitForCatalogManager() call in there, so why would
a subsequent GetTabletLocations RPC return this ServiceUnavailable? As best
I can tell, the only way for this to happen is if the attempt to grab the
leadership lock from within the ListTables RPC (sent from
WaitForCatalogManager()) returns IllegalState, which it'll do if the
UUID in the master's cstate doesn't match the UUID on disk. Perhaps this can
happen during a leader master election; maybe the cstate's UUID becomes
empty for a little while? If that's true, this should fix the problem by
considering IllegalState to be a non-final state and continuing the loop.

I couldn't repro this failure, but Alexey managed to do so in a dist-test
loop with special latency injection enabled. Without the fix, 93 out of 256
runs failed, and with the fix, none failed.

Change-Id: I8192bd669e7e309943ea82718dd715238d520bbd
Reviewed-on: http://gerrit.cloudera.org:8080/11918
Reviewed-by: Alexey Serbin 
Tested-by: Kudu Jenkins
---
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
3 files changed, 21 insertions(+), 8 deletions(-)

Approvals:
  Alexey Serbin: Looks good to me, approved
  Kudu Jenkins: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8192bd669e7e309943ea82718dd715238d520bbd
Gerrit-Change-Number: 11918
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)


[kudu-CR] raft consensus nonvoter-itest: deflake a bit

2018-11-14 Thread Adar Dembo (Code Review)
Hello Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: raft_consensus_nonvoter-itest: deflake a bit
..

raft_consensus_nonvoter-itest: deflake a bit

I saw a failure in ReplicaBehindWalGcThresholdITest.ReplicaReplacement
(GetParam() was (1, false)) just after the master was restarted:

  raft_consensus_nonvoter-itest.cc:2070: Failure
  Failed
  Bad status: Service unavailable: Leader not yet ready to serve requests

This is odd as there's a WaitForCatalogManager() call in there, so why would
a subsequent GetTabletLocations RPC return this ServiceUnavailable? As best
I can tell, the only way for this to happen is if the attempt to grab the
leadership lock from within the ListTables RPC (sent from
WaitForCatalogManager()) returns IllegalState, which it'll do if the
UUID in the master's cstate doesn't match the UUID on disk. Perhaps this can
happen during a leader master election; maybe the cstate's UUID becomes
empty for a little while? If that's true, this should fix the problem by
considering IllegalState to be a non-final state and continuing the loop.

I couldn't repro this failure, but Alexey managed to do so in a dist-test
loop with special latency injection enabled. Without the fix, 93 out of 256
runs failed, and with the fix, none failed.

Change-Id: I8192bd669e7e309943ea82718dd715238d520bbd
---
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
3 files changed, 21 insertions(+), 8 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8192bd669e7e309943ea82718dd715238d520bbd
Gerrit-Change-Number: 11918
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)


[kudu-CR] test: deflake TestRestartWithPendingCommitFromFailedOp

2018-11-14 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11929 )

Change subject: test: deflake TestRestartWithPendingCommitFromFailedOp
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic76b5f120aee7327f6cee7324b862bb89c5662ab
Gerrit-Change-Number: 11929
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 14 Nov 2018 17:57:49 +
Gerrit-HasComments: No


[kudu-CR] test: deflake TestRestartWithPendingCommitFromFailedOp

2018-11-14 Thread Adar Dembo (Code Review)
Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/11929 )

Change subject: test: deflake TestRestartWithPendingCommitFromFailedOp
..

test: deflake TestRestartWithPendingCommitFromFailedOp

The test would fail to create the tablet due to the injected failure,
when it should have really waited until the tablet was bootstrapped
before injecting errors.

The test failed 6/1000 times with stress and with a seed of -922249903.
With the fix, 1000/1000 passed.

This isn't the only source of flakiness in ts_recovery-itest, but it's
something.

Change-Id: Ic76b5f120aee7327f6cee7324b862bb89c5662ab
Reviewed-on: http://gerrit.cloudera.org:8080/11929
Tested-by: Andrew Wong 
Reviewed-by: Alexey Serbin 
Reviewed-by: Adar Dembo 
---
M src/kudu/integration-tests/ts_recovery-itest.cc
1 file changed, 2 insertions(+), 3 deletions(-)

Approvals:
  Andrew Wong: Verified
  Alexey Serbin: Looks good to me, approved
  Adar Dembo: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic76b5f120aee7327f6cee7324b862bb89c5662ab
Gerrit-Change-Number: 11929
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] [location awareness] Register client with location when connect to cluster

2018-11-13 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11923 )

Change subject: [location_awareness] Register client with location when connect 
to cluster
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11923/5/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:

http://gerrit.cloudera.org:8080/#/c/11923/5/src/kudu/master/master_service.cc@537
PS5, Line 537:   Status s = TSDescriptor::RegisterNew(instance, registration, 
);
This seems like the most significant aspect of the change, and should be pulled 
out into its own commit so it can be properly discussed. What's the motivation 
for treating the client as a tserver and "registering" it with the master vs. 
refactoring the location-assigning machinery so that clients can use it without 
registering?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a
Gerrit-Change-Number: 11923
Gerrit-PatchSet: 5
Gerrit-Owner: Fengling Wang 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Tue, 13 Nov 2018 19:14:27 +
Gerrit-HasComments: Yes


[kudu-CR] [test] Clear the KuduClientCache between tests

2018-11-13 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11924 )

Change subject: [test] Clear the KuduClientCache between tests
..


Patch Set 1: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11924/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11924/1//COMMIT_MSG@10
PS1, Line 10: If the cache
: is not cleared between tests, client state and
: configurations could carry over from another test
: and cause failures that are difficult to debug.
I think this change is a good idea, but do we have any proof of this?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d989c00f9b0407e1db4b11df68080ef20cd471c
Gerrit-Change-Number: 11924
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 13 Nov 2018 18:45:21 +
Gerrit-HasComments: Yes


[kudu-CR] KUDU-2599: fix flaky DefaultSourceTest.testSocketReadTimeoutPropagation

2018-11-09 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11916 )

Change subject: KUDU-2599: fix flaky 
DefaultSourceTest.testSocketReadTimeoutPropagation
..


Patch Set 1:

> Did you run a dist_test loop to prove this works? I tried this change and 
> still saw flakiness.

I looped all DefaultSourceTest 1000 times with TSAN binaries. All passed.

http://dist-test.cloudera.org/job?job_id=adar.1541790130.63678


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I81a20f776c115caf3bd720bc532bbeb5fc69bc42
Gerrit-Change-Number: 11916
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Fri, 09 Nov 2018 19:47:22 +
Gerrit-HasComments: No


[kudu-CR] raft consensus nonvoter-itest: deflake a bit

2018-11-09 Thread Adar Dembo (Code Review)
Hello Alexey Serbin,

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

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

to review the following change.


Change subject: raft_consensus_nonvoter-itest: deflake a bit
..

raft_consensus_nonvoter-itest: deflake a bit

I saw a failure in ReplicaBehindWalGcThresholdITest.ReplicaReplacement
(GetParam() was (1, false)) just after the master was restarted:

  raft_consensus_nonvoter-itest.cc:2070: Failure
  Failed
  Bad status: Service unavailable: Leader not yet ready to serve requests

This is odd as there's a WaitForCatalogManager() call in there, so why would
a subsequent GetTabletLocations RPC return this ServiceUnavailable? As best
I can tell, the only way for this to happen is if the attempt to grab the
leadership lock from within the ListTables RPC (sent from
WaitForCatalogManager()) returns IllegalState, which it'll do if the
UUID in the master's cstate doesn't match the UUID on disk. Perhaps this can
happen during a leader master election; maybe the cstate's UUID becomes
empty for a little while?

If that's true, this should fix the problem by considering IllegalState to
be a non-final state and continuing the loop.

Change-Id: I8192bd669e7e309943ea82718dd715238d520bbd
---
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
3 files changed, 21 insertions(+), 8 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8192bd669e7e309943ea82718dd715238d520bbd
Gerrit-Change-Number: 11918
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 


[kudu-CR] (05/05) delta applier: support for diff scan style iteration

2018-11-09 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11860 )

Change subject: (05/05) delta_applier: support for diff scan style iteration
..


Patch Set 4: Verified+1

Overriding Jenkins, KUDU-2599 plus another unrelated flake.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I895350a3c7728df88a87a36b904ffdea4f3f6b7a
Gerrit-Change-Number: 11860
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 09 Nov 2018 18:59:53 +
Gerrit-HasComments: No


[kudu-CR] (05/05) delta applier: support for diff scan style iteration

2018-11-09 Thread Adar Dembo (Code Review)
Adar Dembo has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/11860 )

Change subject: (05/05) delta_applier: support for diff scan style iteration
..


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11860
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I895350a3c7728df88a87a36b904ffdea4f3f6b7a
Gerrit-Change-Number: 11860
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] (02/05) delta store: convert DeltaIterator::PrepareBatch flags into bitfield

2018-11-09 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11857 )

Change subject: (02/05) delta_store: convert DeltaIterator::PrepareBatch flags 
into bitfield
..


Patch Set 3: Verified+1

Overriding Jenkins, known KUDU-2599 flake.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id500ec3cc9459a78ae1098be3f3cd0cacb7a7a1a
Gerrit-Change-Number: 11857
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 09 Nov 2018 18:10:13 +
Gerrit-HasComments: No


[kudu-CR] (02/05) delta store: convert DeltaIterator::PrepareBatch flags into bitfield

2018-11-09 Thread Adar Dembo (Code Review)
Adar Dembo has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/11857 )

Change subject: (02/05) delta_store: convert DeltaIterator::PrepareBatch flags 
into bitfield
..


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11857
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Id500ec3cc9459a78ae1098be3f3cd0cacb7a7a1a
Gerrit-Change-Number: 11857
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] (03/05) delta store: support iteration with snap to exclude

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/11858 )

Change subject: (03/05) delta_store: support iteration with snap_to_exclude
..


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11858
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c
Gerrit-Change-Number: 11858
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] (03/05) delta store: support iteration with snap to exclude

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11858 )

Change subject: (03/05) delta_store: support iteration with snap_to_exclude
..


Patch Set 3: Verified+1

Overriding Jenkins, the failure was known flake KUDU-2599.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c
Gerrit-Change-Number: 11858
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 09 Nov 2018 05:52:02 +
Gerrit-HasComments: No


[kudu-CR] KUDU-2599: fix flaky DefaultSourceTest.testSocketReadTimeoutPropagation

2018-11-08 Thread Adar Dembo (Code Review)
Hello Mike Percy, Grant Henke,

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

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

to review the following change.


Change subject: KUDU-2599: fix flaky 
DefaultSourceTest.testSocketReadTimeoutPropagation
..

KUDU-2599: fix flaky DefaultSourceTest.testSocketReadTimeoutPropagation

This test is just about checking the propagation of an option from the
DefaultSource to the KuduRelation. However, building a DataFrame involves a
connection to the cluster, and if we set socketReadTimeoutMs to 1, it's
going to be very difficult to connect, especially if we're testing
TSAN-instrumented binaries whose thread creation is inherently slower.

The fix? Use a magic number larger than '1'. It's that simple.

Change-Id: I81a20f776c115caf3bd720bc532bbeb5fc69bc42
---
M 
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
1 file changed, 8 insertions(+), 4 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I81a20f776c115caf3bd720bc532bbeb5fc69bc42
Gerrit-Change-Number: 11916
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Mike Percy 


[kudu-CR] (03/05) delta store: support iteration with snap to exclude

2018-11-08 Thread Adar Dembo (Code Review)
Hello Mike Percy, David Ribeiro Alves, Kudu Jenkins, Grant Henke, Todd Lipcon,

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

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

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

Change subject: (03/05) delta_store: support iteration with snap_to_exclude
..

(03/05) delta_store: support iteration with snap_to_exclude

This patch changes the delta stores (DMS and delta files) to respect
snap_to_exclude during iteration. The key changes are:
- The introduction of the "selection" criteria, a new delta relevancy
  formula for determining whether a delta applies to a scan with both
  snap_to_exclude and snap_to_include. The existing "application" criteria
  was formalized and moved into delta_relevancy.h. There was also a
  non-trivial change to DeltaFileReader::IsRelevantForSnapshot() to use both
  criterias when culling entire delta files.
- A new SelectUpdates() method for using the selection criteria on a batch
  of prepared deltas. SelectUpdates() requires new in-memory state, the
  creation of which is gated behind a new PREPARE_FOR_SELECT flag so as not
  to affect regular scans.
- Updates to the delta fuzz testing logic to test iterators with two
  timestamps, and to provide SelectUpdates() coverage.

A future patch will modify DeltaApplier to use SelectUpdates for diff scans.

Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c
---
M src/kudu/tablet/delta_iterator_merger.cc
M src/kudu/tablet/delta_iterator_merger.h
A src/kudu/tablet/delta_relevancy.h
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
M src/kudu/tablet/tablet-test-util.cc
M src/kudu/tablet/tablet-test-util.h
11 files changed, 468 insertions(+), 92 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c
Gerrit-Change-Number: 11858
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] (02/05) delta store: convert DeltaIterator::PrepareBatch flags into bitfield

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11857 )

Change subject: (02/05) delta_store: convert DeltaIterator::PrepareBatch flags 
into bitfield
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11857/2/src/kudu/tablet/delta_store.h
File src/kudu/tablet/delta_store.h:

http://gerrit.cloudera.org:8080/#/c/11857/2/src/kudu/tablet/delta_store.h@205
PS2, Line 205:   // 'flags' is a bitfield describing what operation(s) the 
batch will be used
> Nit: Is there a more descriptive name that could be used? prepairForFlags?
Sure, I can rename it to something more specific.


http://gerrit.cloudera.org:8080/#/c/11857/2/src/kudu/tablet/delta_store.h@349
PS2, Line 349: prepared_for
> prepared_flags_
Done


http://gerrit.cloudera.org:8080/#/c/11857/2/src/kudu/tablet/delta_store.h@369
PS2, Line 369: prepared_for_
> same
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id500ec3cc9459a78ae1098be3f3cd0cacb7a7a1a
Gerrit-Change-Number: 11857
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 09 Nov 2018 04:58:40 +
Gerrit-HasComments: Yes


[kudu-CR] (02/05) delta store: convert DeltaIterator::PrepareBatch flags into bitfield

2018-11-08 Thread Adar Dembo (Code Review)
Hello Mike Percy, David Ribeiro Alves, Grant Henke, Todd Lipcon,

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

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

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

Change subject: (02/05) delta_store: convert DeltaIterator::PrepareBatch flags 
into bitfield
..

(02/05) delta_store: convert DeltaIterator::PrepareBatch flags into bitfield

A follow-on patch will introduce DeltaIterator::SelectUpdates(), a new
method for extracting delta information from a DeltaIterator. As it is
only to be used by diff scans and will require dedicated in-memory state,
it'll also come with a new DeltaIterator::PrepareBatch() flag.

However, diff scans will need to use both SelectUpdates() and
ApplyUpdates(), and it'd be a shame to have to reseek and reprepare just to
use both extraction methods. As such, this patch makes it possible to
prepare a batch for use in multiple delta extraction methods.

Change-Id: Id500ec3cc9459a78ae1098be3f3cd0cacb7a7a1a
---
M src/kudu/tablet/delta_iterator_merger.cc
M src/kudu/tablet/delta_iterator_merger.h
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
M src/kudu/tablet/tablet-test-util.h
9 files changed, 55 insertions(+), 80 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id500ec3cc9459a78ae1098be3f3cd0cacb7a7a1a
Gerrit-Change-Number: 11857
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] (01/05) delta store: avoid copying deleted row data in ApplyUpdates

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/11856 )

Change subject: (01/05) delta_store: avoid copying deleted row data in 
ApplyUpdates
..

(01/05) delta_store: avoid copying deleted row data in ApplyUpdates

When applying deltas, the scan path will first populate a selection vector
with deletes (i.e. a row is unset if there's a relevant DELETE for it), and
then use it to enable two optimizations:
1. Short-circuit out if all rows in the block have been deleted.
2. Skip predicate evaluation and base data copying for deleted rows.

To this we can add a third optimization: don't apply a delta whose mutation
is to a deleted row. Note that it's not incorrect to apply such deltas (as
we'll skip deleted rows when serializing the scan response to the client);
it's just unnecessary work.

I wrote a microbenchmark to evaluate the impact of this optimization. Below
is the timing and perf stat output of the microbenchmark before and after
applying the optimization (specifically, applying this patch, then
commenting out the filtering in DeltaPrepare::ApplyUpdates for the first
run, and uncommenting it for the next run).

  Time spent running 1000 scans with 0 deletes: real 0.523s user 0.524s 
sys 0.000s
  Time spent running 1000 scans with 10 deletes: real 0.515suser 0.516s 
sys 0.000s
  Time spent running 1000 scans with 100 deletes: real 0.512s   user 0.512s 
sys 0.000s
  Time spent running 1000 scans with 1000 deletes: real 0.553s  user 0.552s 
sys 0.000s

   Performance counter stats for 'bin/deltamemstore-test 
--gtest_filter=*Varying* --benchmark_num_passes=1000':

   2201.029290  task-clock (msec) #0.991 CPUs utilized
 5  context-switches  #0.002 K/sec
 0  cpu-migrations#0.000 K/sec
 4,950  page-faults   #0.002 M/sec
 8,276,723,832  cycles#3.760 GHz
24,539,935,941  instructions  #2.96  insn per cycle
 4,709,709,705  branches  # 2139.776 M/sec
12,631,579  branch-misses #0.27% of all branches

   2.220370506 seconds time elapsed

  Time spent running 1000 scans with 0 deletes: real 0.474s user 0.475s 
sys 0.000s
  Time spent running 1000 scans with 10 deletes: real 0.475suser 0.472s 
sys 0.004s
  Time spent running 1000 scans with 100 deletes: real 0.478s   user 0.476s 
sys 0.004s
  Time spent running 1000 scans with 1000 deletes: real 0.550s  user 0.552s 
sys 0.000s

   Performance counter stats for 'bin/deltamemstore-test 
--gtest_filter=*Varying* --benchmark_num_passes=1000':

   2074.795741  task-clock (msec) #0.990 CPUs utilized
23  context-switches  #0.011 K/sec
 1  cpu-migrations#0.000 K/sec
 4,951  page-faults   #0.002 M/sec
 7,675,100,058  cycles#3.699 GHz
23,100,692,252  instructions  #3.01  insn per cycle
 4,539,777,117  branches  # 2188.060 M/sec
11,819,267  branch-misses #0.26% of all branches

   2.096193851 seconds time elapsed

Note: I originally wrote this patch thinking it was necessary for diff scan
correctness, but have since convinced myself that it's just an optimization.

Change-Id: I6a646d0816a96e9aba486c0d0a1e6b4a7e15c144
Reviewed-on: http://gerrit.cloudera.org:8080/11856
Tested-by: Kudu Jenkins
Reviewed-by: Grant Henke 
Reviewed-by: Mike Percy 
---
M src/kudu/tablet/delta_applier.cc
M src/kudu/tablet/delta_applier.h
M src/kudu/tablet/delta_iterator_merger.cc
M src/kudu/tablet/delta_iterator_merger.h
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
M src/kudu/tablet/tablet-test-util.h
13 files changed, 116 insertions(+), 46 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Grant Henke: Looks good to me, but someone else must approve
  Mike Percy: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6a646d0816a96e9aba486c0d0a1e6b4a7e15c144
Gerrit-Change-Number: 11856
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] KUDU-2378: fix another aligned load crash

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/11911 )

Change subject: KUDU-2378: fix another aligned load crash
..

KUDU-2378: fix another aligned load crash

The TestKuduBackup.testRandomBackupAndRestore test from the kudu-backup
project would crash a TSAN master from time to time, with a stack trace
ending in Variant::Reset. After some debugging, this turned out to be a
holdover from KUDU-2378, where clang emitted a movaps (Aligned Load)
instruction on a value that wasn't aligned. The holdover was missed
because it was a static_cast (the original search covered
reinterpret_casts); I did a search for other bad casts but found none.

Why TSAN? Probably because it's the only build to use libc++, whose
std::string uses the Small String Optimization to embed the data into the
string itself, thus raising the possibility of the string's data being
unaligned. By comparison, libstdc++'s COW std::string uses a separate
allocation for string data, which is likely always 16-byte aligned.

The patch includes a unit test that would repro the crash 100% of the time
when built for TSAN.

Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357
Reviewed-on: http://gerrit.cloudera.org:8080/11911
Tested-by: Adar Dembo 
Reviewed-by: Todd Lipcon 
---
M src/kudu/common/types.h
M src/kudu/common/wire_protocol-test.cc
2 files changed, 11 insertions(+), 1 deletion(-)

Approvals:
  Adar Dembo: Verified
  Todd Lipcon: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357
Gerrit-Change-Number: 11911
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] KUDU-2378: fix another aligned load crash

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11911 )

Change subject: KUDU-2378: fix another aligned load crash
..


Patch Set 4: Verified+1

Overriding Jenkins, DefaultSourceTest failed in TSAN again.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357
Gerrit-Change-Number: 11911
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 09 Nov 2018 01:03:26 +
Gerrit-HasComments: No


[kudu-CR] KUDU-2378: fix another aligned load crash

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/11911 )

Change subject: KUDU-2378: fix another aligned load crash
..


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11911
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357
Gerrit-Change-Number: 11911
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] KUDU-2378: fix another aligned load crash

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11911 )

Change subject: KUDU-2378: fix another aligned load crash
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11911/2/src/kudu/common/types.h
File src/kudu/common/types.h:

http://gerrit.cloudera.org:8080/#/c/11911/2/src/kudu/common/types.h@702
PS2, Line 702: numeric_.u64 = *static_cast(value);
> mind fixing all these others, too, so that whenever some poor soul tries to
Todd and I talked offline. He started on a patch to fix all aligned loads, but 
decided it wasn't a high priority fix and shelved it. He gave his blessing to 
this patch which just fixes the int128 case, which is actually critical for x86.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357
Gerrit-Change-Number: 11911
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 08 Nov 2018 23:40:33 +
Gerrit-HasComments: Yes


[kudu-CR] [java] Upgrade to Spark 2.4

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11912 )

Change subject: [java] Upgrade to Spark 2.4
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id1f8de543276c4dc82a57c4a2228ae2374f2d87f
Gerrit-Change-Number: 11912
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 08 Nov 2018 23:42:13 +
Gerrit-HasComments: No


[kudu-CR] KUDU-2378: fix another aligned load crash

2018-11-08 Thread Adar Dembo (Code Review)
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Grant Henke, Todd Lipcon,

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

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

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

Change subject: KUDU-2378: fix another aligned load crash
..

KUDU-2378: fix another aligned load crash

The TestKuduBackup.testRandomBackupAndRestore test from the kudu-backup
project would crash a TSAN master from time to time, with a stack trace
ending in Variant::Reset. After some debugging, this turned out to be a
holdover from KUDU-2378, where clang emitted a movaps (Aligned Load)
instruction on a value that wasn't aligned. The holdover was missed
because it was a static_cast (the original search covered
reinterpret_casts); I did a search for other bad casts but found none.

Why TSAN? Probably because it's the only build to use libc++, whose
std::string uses the Small String Optimization to embed the data into the
string itself, thus raising the possibility of the string's data being
unaligned. By comparison, libstdc++'s COW std::string uses a separate
allocation for string data, which is likely always 16-byte aligned.

The patch includes a unit test that would repro the crash 100% of the time
when built for TSAN.

Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357
---
M src/kudu/common/types.h
M src/kudu/common/wire_protocol-test.cc
2 files changed, 11 insertions(+), 1 deletion(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357
Gerrit-Change-Number: 11911
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] KUDU-2378: fix another aligned load crash

2018-11-08 Thread Adar Dembo (Code Review)
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Grant Henke, Todd Lipcon,

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

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

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

Change subject: KUDU-2378: fix another aligned load crash
..

KUDU-2378: fix another aligned load crash

The TestKuduBackup.testRandomBackupAndRestore test from the kudu-backup
project would crash a TSAN master from time to time, with a stack trace
ending in Variant::Reset. After some debugging, this turned out to be a
holdover from KUDU-2378, where clang emitted a movaps (Aligned Load)
instruction on a value that wasn't aligned. The holdover was missed
because it was a static_cast (the original search covered
reinterpret_casts); I did a search for other bad casts but found none.

Why TSAN? Probably because it's the only build to use libc++, whose
std::string uses the Small String Optimization to embed the data into the
string itself, thus raising the possibility of the string's data being
unaligned. By comparison, libstdc++'s COW std::string uses a separate
allocation for string data, which is likely always 16-byte aligned.

The patch includes a unit test that would repro the crash 100% of the time
when built for TSAN.

Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357
---
M src/kudu/common/types.h
M src/kudu/common/wire_protocol-test.cc
2 files changed, 10 insertions(+), 1 deletion(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357
Gerrit-Change-Number: 11911
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] KUDU-2378: fix another aligned load crash

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11911 )

Change subject: KUDU-2378: fix another aligned load crash
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11911/2/src/kudu/common/wire_protocol-test.cc
File src/kudu/common/wire_protocol-test.cc:

http://gerrit.cloudera.org:8080/#/c/11911/2/src/kudu/common/wire_protocol-test.cc@455
PS2, Line 455:   pb.set_read_default_value(string(16, 'a'));
> warning: length is bigger then string literal size [bugprone-string-constru
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357
Gerrit-Change-Number: 11911
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 08 Nov 2018 22:23:50 +
Gerrit-HasComments: Yes


[kudu-CR] KUDU-2378: fix another aligned load crash

2018-11-08 Thread Adar Dembo (Code Review)
Hello Alexey Serbin, Kudu Jenkins, Grant Henke, Todd Lipcon,

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

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

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

Change subject: KUDU-2378: fix another aligned load crash
..

KUDU-2378: fix another aligned load crash

The TestKuduBackup.testRandomBackupAndRestore test from the kudu-backup
project would crash a TSAN master from time to time, with a stack trace
ending in Variant::Reset. After some debugging, this turned out to be a
holdover from KUDU-2378, where clang emitted a movaps (Aligned Load)
instruction on a value that wasn't aligned. The holdover was missed
because it was a static_cast (the original search covered
reinterpret_casts); I did a search for other bad casts but found none.

Why TSAN? Probably because it's the only build to use libc++, whose
std::string uses the Small String Optimization to embed the data into the
string itself, thus raising the possibility of the string's data being
unaligned. By comparison, libstdc++'s COW std::string uses a separate
allocation for string data, which is likely always 16-byte aligned.

The patch includes a unit test that would repro the crash 100% of the time
when built for TSAN.

Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357
---
M src/kudu/common/types.h
M src/kudu/common/wire_protocol-test.cc
2 files changed, 10 insertions(+), 1 deletion(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357
Gerrit-Change-Number: 11911
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] run-test: don't report leaks that don't fail test

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11886 )

Change subject: run-test: don't report leaks that don't fail test
..


Patch Set 4:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/11886/4/src/kudu/server/generic_service.cc@183
PS4, Line 183: LOG(INFO) << "LeakSanitizer found false positives. 
Ignoring them.";
May want to make this bigger so it stands out from the report. And maybe 
WARNING instead of INFO.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 07 Nov 2018 23:50:37 +
Gerrit-HasComments: Yes


[kudu-CR] gradle: expose testRandomSeed property

2018-11-08 Thread Adar Dembo (Code Review)
Hello Grant Henke,

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

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

to review the following change.


Change subject: gradle: expose testRandomSeed property
..

gradle: expose testRandomSeed property

In order to read a system property in tests, it isn't enough to add a
System.getProperty() call to the test; we need to expose the property in
gradle so that it's properly passed through to the test. With this patch,
testRandomSeed can be set using either -D or -P.

Note that even with testRandomSeed overridden, the PRNG didn't produce
deterministic results in TestKuduBackup.testRandomBackupAndRestore, but
perhaps that's a quirk of Scala's Random class.

Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d
---
M java/gradle/tests.gradle
1 file changed, 5 insertions(+), 0 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d
Gerrit-Change-Number: 11908
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Grant Henke 


[kudu-CR] [dist-test] build environment for tests in loop mode

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11905 )

Change subject: [dist-test] build environment for tests in loop mode
..


Patch Set 4: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d
Gerrit-Change-Number: 11905
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 08 Nov 2018 20:10:01 +
Gerrit-HasComments: No


[kudu-CR] KUDU-2378: fix another aligned load crash

2018-11-08 Thread Adar Dembo (Code Review)
Hello Alexey Serbin, Grant Henke, Todd Lipcon,

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

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

to review the following change.


Change subject: KUDU-2378: fix another aligned load crash
..

KUDU-2378: fix another aligned load crash

The TestKuduBackup.testRandomBackupAndRestore test from the kudu-backup
project would crash a TSAN master from time to time, with a stack trace
ending in Variant::Reset. After some debugging, this turned out to be a
holdover from KUDU-2378, where clang emitted a movaps (Aligned Load)
instruction on a value that wasn't aligned. The holdover was missed
because it was a static_cast (the original search covered
reinterpret_casts); I did a search for other bad casts but found none.

Why TSAN? Probably because it's the only build to use libc++, whose
std::string uses the Small String Optimization to embed the data into the
string itself, thus raising the possibility of the string's data being
unaligned. By comparison, libstdc++'s COW std::string uses a separate
allocation for string data, which is likely always 16-byte aligned.

The patch includes a unit test that isn't totally necessary, but was a
useful way to repro the crash in relative isolation.

Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357
---
M src/kudu/common/types.h
M src/kudu/master/master-test.cc
2 files changed, 21 insertions(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I13e23a9d88db878071faeff7b9de06d1bdd98357
Gerrit-Change-Number: 11911
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] gradle: expose testRandomSeed property

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11908 )

Change subject: gradle: expose testRandomSeed property
..


Patch Set 2: Verified+1

Overriding Jenkins, DefaultSourceTest failed three times in TSAN.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d
Gerrit-Change-Number: 11908
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 08 Nov 2018 19:28:05 +
Gerrit-HasComments: No


[kudu-CR] gradle: expose testRandomSeed property

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/11908 )

Change subject: gradle: expose testRandomSeed property
..

gradle: expose testRandomSeed property

In order to read a system property in tests, it isn't enough to add a
System.getProperty() call to the test; we need to expose the property in
gradle so that it's properly passed through to the test. With this patch,
testRandomSeed can be set using either -D or -P.

Note that even with testRandomSeed overridden, the PRNG didn't produce
deterministic results in TestKuduBackup.testRandomBackupAndRestore, but
perhaps that's a quirk of Scala's Random class.

Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d
Reviewed-on: http://gerrit.cloudera.org:8080/11908
Reviewed-by: Grant Henke 
Tested-by: Adar Dembo 
---
M java/gradle/tests.gradle
1 file changed, 7 insertions(+), 0 deletions(-)

Approvals:
  Grant Henke: Looks good to me, approved
  Adar Dembo: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d
Gerrit-Change-Number: 11908
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 


[kudu-CR] gradle: expose testRandomSeed property

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/11908 )

Change subject: gradle: expose testRandomSeed property
..


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d
Gerrit-Change-Number: 11908
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 


[kudu-CR] Add "network plane" as part of ConnectionId

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "network_plane" as part of ConnectionId
..

Add "network_plane" as part of ConnectionId

The motivation for doing so is to allow N services on the same host to
be multiplexed on M different connections. For instance, a server may
host multiple KRPC based services: one for control command and one for
data transfer. Separating the connections between the control channel
and the data channel prevents unnecessary delays of the control commands
due to being stuck behind large data transfers from client to server.

By default, the network_plane of a new ConnectionId is not set.
A user can change it to a different value by calling
Proxy::set_network_plane() on the ConnectionId.

Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Reviewed-on: http://gerrit.cloudera.org:8080/11681
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
---
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/messenger.h
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/proxy.h
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
7 files changed, 138 insertions(+), 31 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] gradle: expose testRandomSeed property

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11908 )

Change subject: gradle: expose testRandomSeed property
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11908/1/java/gradle/tests.gradle
File java/gradle/tests.gradle:

http://gerrit.cloudera.org:8080/#/c/11908/1/java/gradle/tests.gradle@69
PS1, Line 69:   if (propertyExists("testRandomSeed")) {
> This can't default to "null". It results in a NumberFormatException when tr
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d
Gerrit-Change-Number: 11908
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 08 Nov 2018 18:58:23 +
Gerrit-HasComments: Yes


[kudu-CR] gradle: expose testRandomSeed property

2018-11-08 Thread Adar Dembo (Code Review)
Hello Kudu Jenkins, Grant Henke,

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

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

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

Change subject: gradle: expose testRandomSeed property
..

gradle: expose testRandomSeed property

In order to read a system property in tests, it isn't enough to add a
System.getProperty() call to the test; we need to expose the property in
gradle so that it's properly passed through to the test. With this patch,
testRandomSeed can be set using either -D or -P.

Note that even with testRandomSeed overridden, the PRNG didn't produce
deterministic results in TestKuduBackup.testRandomBackupAndRestore, but
perhaps that's a quirk of Scala's Random class.

Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d
---
M java/gradle/tests.gradle
1 file changed, 7 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4873d27998f770a45dd9cc85f84ec8c146261b3d
Gerrit-Change-Number: 11908
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)


[kudu-CR] [backup] Ensure random tests are reproducible

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11909 )

Change subject: [backup] Ensure random tests are reproducible
..


Patch Set 2: Code-Review+2

So what PRNG instance do Random.nextFoo() calls use?


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f376ba5709ec118c4327881f7c864b23f92ac4d
Gerrit-Change-Number: 11909
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 08 Nov 2018 18:44:39 +
Gerrit-HasComments: No


[kudu-CR] [cmake] no empty KUDU DATA FILES ctest env variables

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11907 )

Change subject: [cmake] no empty KUDU_DATA_FILES ctest env variables
..


Patch Set 4: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb
Gerrit-Change-Number: 11907
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 08 Nov 2018 08:32:46 +
Gerrit-HasComments: No


[kudu-CR] [cmake] no empty KUDU DATA FILES ctest env variables

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11907 )

Change subject: [cmake] no empty KUDU_DATA_FILES ctest env variables
..


Patch Set 3: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11907/3/CMakeLists.txt
File CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/11907/3/CMakeLists.txt@877
PS3, Line 877: if (NOT "${DATA_FILES_LIST}" STREQUAL "")
Does this work?

  if (NOT DATA_FILES_LIST)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb
Gerrit-Change-Number: 11907
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 08 Nov 2018 00:34:41 +
Gerrit-HasComments: Yes


[kudu-CR] [dist-test] build environment for tests in loop mode

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11905 )

Change subject: [dist-test] build environment for tests in loop mode
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11905/2/build-support/dist_test.py
File build-support/dist_test.py:

http://gerrit.cloudera.org:8080/#/c/11905/2/build-support/dist_test.py@528
PS2, Line 528:   # The presence of the --gtest_filter flag means running some 
particular test
 :   # scenarios provided by the binary. In that case it doesn't 
make much sense
 :   # to shard the execution since only the shards containing the 
matching tests
 :   # scenarios would produce any meaningful results. Also, it's 
not trivial
 :   # to point which particular scenario is run by particular 
shard anyway.
 :   # So, simply disable sharding in that case.
Nit: reword? Maybe "The presence of the --gtest_filter flag means the user is 
interested in a particular subset of tests provided by the binary. In that case 
it doesn't make sense to shard the execution since only the shards containing 
the matching tests would produce interesting results, while the rest would 
yield noise. To avoid this, we disable sharding in this case."



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d
Gerrit-Change-Number: 11905
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 08 Nov 2018 08:32:27 +
Gerrit-HasComments: Yes


[kudu-CR] Add "network plane" as part of ConnectionId

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "network_plane" as part of ConnectionId
..


Patch Set 5: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG@7
PS1, Line 7: Add "network_plane" as part of ConnectionId
> Specifying it per service seems a bit rigid as it hardcodes the data plane
OK, thanks for the explanation. I think Todd's justification for per-RPC might 
hinge on the fact that, within Kudu, we generally use one proxy for every 
remote+service pair. So if the service plane is a property of the proxy, we'd 
need to establish multiple (probably just two) proxies for each remote+service 
pair.

Anyway I don't have a strong feeling about this, and judging by Todd's earlier 
suggestions, neither does he.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 08 Nov 2018 00:33:46 +
Gerrit-HasComments: Yes


[kudu-CR] run-test: don't report leaks that don't fail test

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11886 )

Change subject: run-test: don't report leaks that don't fail test
..


Patch Set 5: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 08 Nov 2018 00:48:38 +
Gerrit-HasComments: No


[kudu-CR] [cmake] no empty KUDU DATA FILES ctest env variables

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11907 )

Change subject: [cmake] no empty KUDU_DATA_FILES ctest env variables
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11907/2/CMakeLists.txt
File CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/11907/2/CMakeLists.txt@876
PS2, Line 876: If the DATA_FILES_LIST is
 : # empty, do not add the KUDU_DATA_FILES environment variable.
FWIW, I think this is obvious from the code.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b255f6148649cd282bab70fb19c3321fec01adb
Gerrit-Change-Number: 11907
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 08 Nov 2018 00:27:37 +
Gerrit-HasComments: Yes


[kudu-CR] run-test: don't report leaks that don't fail test

2018-11-08 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11886 )

Change subject: run-test: don't report leaks that don't fail test
..


Patch Set 6: Code-Review+2

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/11886/5/src/kudu/server/generic_service.cc@52
PS5, Line 52:
> Done
Odd suggestion, seeing as you were clearly using it.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 08 Nov 2018 01:17:08 +
Gerrit-HasComments: Yes


[kudu-CR] run-test: don't report leaks that don't fail test

2018-11-07 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11886 )

Change subject: run-test: don't report leaks that don't fail test
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11886/3/src/kudu/server/generic_service.cc
File src/kudu/server/generic_service.cc:

http://gerrit.cloudera.org:8080/#/c/11886/3/src/kudu/server/generic_service.cc@182
PS3, Line 182:   LOG(INFO) << "LeakSanitizer found false positives. 
Ignoring them.";
Won't this print in the normal case where there are no leaks at all? Seems like 
you need to detect a true->false transition here (where the initial true state 
from L178 doesn't count) and only LOG then.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 07 Nov 2018 22:54:03 +
Gerrit-HasComments: Yes


[kudu-CR] [dist-test] build environment for tests in loop mode

2018-11-07 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11905 )

Change subject: [dist-test] build environment for tests in loop mode
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11905/1/build-support/dist_test.py
File build-support/dist_test.py:

http://gerrit.cloudera.org:8080/#/c/11905/1/build-support/dist_test.py@167
PS1, Line 167:   Return an array of TestExecution objects.
Please doc tests_regex and extra_args.


http://gerrit.cloudera.org:8080/#/c/11905/1/build-support/dist_test.py@170
PS1, Line 170:   ctest_argv = [ctest_bin, "-V", "-N", "-LE", "no_dist_test"]
I ran this command and was surprised to see that we emit KUDU_DATA_FILES into 
the test environment even when there's no DATA_FILES entries. That appears to 
be due to the unconditional use of DATA_FILES_LIST in the top-level 
CMakeLists.txt. Could you fix that?


http://gerrit.cloudera.org:8080/#/c/11905/1/build-support/dist_test.py@517
PS1, Line 517:   # Build the list of executions corresponding to the test. It's 
better to do
Won't this also shard the test? Seems like the epilog for 'loop' would need to 
be updated, at a minimum.

You should read 
https://gerrit.cloudera.org/c/9470/2/build-support/dist_test.py#442 before 
proceeding. I'm worried this change might break the "loop 
build/latest/bin/foo-test" workflow; does it?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I487194a396635bbaab457795bb24c0063eebbe5d
Gerrit-Change-Number: 11905
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 07 Nov 2018 22:50:22 +
Gerrit-HasComments: Yes


[kudu-CR] De-flake sentry client-test and sentry authz provider-test

2018-11-07 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11898 )

Change subject: De-flake sentry_client-test and sentry_authz_provider-test
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e30ce88b7c2ab4afe451c50e13ef739237709e0
Gerrit-Change-Number: 11898
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 07 Nov 2018 17:30:29 +
Gerrit-HasComments: No


[kudu-CR] python: make test scantoken more robust to exceptions

2018-11-07 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11863 )

Change subject: python: make test_scantoken more robust to exceptions
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11863/1/python/kudu/tests/test_scantoken.py
File python/kudu/tests/test_scantoken.py:

http://gerrit.cloudera.org:8080/#/c/11863/1/python/kudu/tests/test_scantoken.py@56
PS1, Line 56: results = pool.map(_get_scan_token_results, input)
> Think it's worth printing the exception too?
The raised exception will continue to go up the stack and will be surfaced by 
the test runner.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9b5a70d6b5f115cc4de740ded6ecc48574b9c707
Gerrit-Change-Number: 11863
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 07 Nov 2018 17:32:23 +
Gerrit-HasComments: Yes


[kudu-CR] python: make test scantoken more robust to exceptions

2018-11-07 Thread Adar Dembo (Code Review)
Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/11863 )

Change subject: python: make test_scantoken more robust to exceptions
..

python: make test_scantoken more robust to exceptions

One of my precommit tests failed with:

  22:49:05 kudu/tests/test_client.py .
  22:49:06 kudu/tests/test_scanner.py 
  22:49:08 kudu/tests/test_scantoken.py ..Build timed out (after 50 minutes). 
Marking the build as failed.
  23:39:08 Build was aborted

I don't know why this happened, but looking at commit 1d928951e, it seems
like if pool.map() raised an exception, we wouldn't close or join the pool,
which would lead to the same test hanging behavior as before. I don't know
how or why pool.map() raised an exception, but this will hopefully surface
the actual root cause.

Change-Id: I9b5a70d6b5f115cc4de740ded6ecc48574b9c707
Reviewed-on: http://gerrit.cloudera.org:8080/11863
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong 
---
M python/kudu/tests/test_scantoken.py
1 file changed, 5 insertions(+), 3 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Andrew Wong: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9b5a70d6b5f115cc4de740ded6ecc48574b9c707
Gerrit-Change-Number: 11863
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Jordan Birdsell 
Gerrit-Reviewer: Kudu Jenkins (120)


[kudu-CR] run-test: don't report leaks that don't fail test

2018-11-07 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11886 )

Change subject: run-test: don't report leaks that don't fail test
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11886/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11886/2//COMMIT_MSG@16
PS2, Line 16: We currently inject errors into JUnit XML report if any leaks are 
found.
: This patch updates this to only inject these errors if the test 
also
: failed.
> Ah, I like that approach much better. You're right that it isn't clear why
Oh, so this is squarely the "false positive leak that goes away if 
__lsan_do_recoverable_leak_check() is called again" issue? I didn't realize 
LSAN still prints a leak report in that case, but I guess that's to be 
expected. Unfortunately I don't see a way to control the printing of the report 
at runtime within LSAN.

In that case, maybe your original solution is the correct one. Maybe we should 
also print something in GenericServiceImpl::CheckLeaks to the effect of "LSAN 
found false positives, ignore them".



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 07 Nov 2018 17:29:41 +
Gerrit-HasComments: Yes


[kudu-CR] run-test: don't report leaks that don't fail test

2018-11-06 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11886 )

Change subject: run-test: don't report leaks that don't fail test
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11886/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11886/2//COMMIT_MSG@12
PS2, Line 12: test
Nit: tests


http://gerrit.cloudera.org:8080/#/c/11886/2//COMMIT_MSG@16
PS2, Line 16: We currently inject errors into JUnit XML report if any leaks are 
found.
: This patch updates this to only inject these errors if the test 
also
: failed.
Hmm, I don't really understand why this is safe. Are you 100% sure that a leak 
without a corresponding test failure constitutes a false positive? What 
evidence supports this?

Have you tried something different, such as adding a ScopedLeakCheckDisabler to 
the string allocation within Thread::SuperviseThread? Or, in 
GenericServiceImpl::CheckLeaks, either looping further or sleeping for longer 
in between loop cycles?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1f199795c48bd9b6106110aae132ec165eb0f647
Gerrit-Change-Number: 11886
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 07 Nov 2018 00:23:49 +
Gerrit-HasComments: Yes


[kudu-CR] Add "network plane" as part of ConnectionId

2018-11-06 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "network_plane" as part of ConnectionId
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11681/1//COMMIT_MSG@7
PS1, Line 7: Add "network_plane" as part of ConnectionId
> Sorry for the delay. I adopted option 1. Thanks.
I'm curious to hear your thinking on, when implementing option 1, you did it 
per-Proxy instead of per-RPC as Todd had suggested?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 07 Nov 2018 00:08:46 +
Gerrit-HasComments: Yes


[kudu-CR] Add "network plane" as part of ConnectionId

2018-11-06 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11681 )

Change subject: Add "network_plane" as part of ConnectionId
..


Patch Set 4:

(8 comments)

Could you also update this comment in reactor.h:

  // Client-side connection map. Multiple connections could be open to a remote
  // server if multiple credential policies are used for individual RPCs.

While true, the wording should be made more generic since a connection ID is 
now more than just a remote and set of credentials.

http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/connection_id.cc
File src/kudu/rpc/connection_id.cc:

http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/connection_id.cc@50
PS4, Line 50: void ConnectionId::set_network_plane(std::string network_plane) {
Nit: we're using std::string here; don't need std:: prefix.


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/connection_id.cc@62
PS4, Line 62:   return strings::Substitute("{remote=$0, user_credentials=$1, 
network_plane=$2}",
Maybe omit the plane altogether if it's the default one?


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h
File src/kudu/rpc/proxy.h:

http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@54
PS4, Line 54: so higher
Nit: "so that a higher"


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@55
PS4, Line 55: control channel
Nit: "a control channel"


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/proxy.h@118
PS4, Line 118:   void set_network_plane(const std::string& network_plane);
Could we pass by copy here and std::move into the underlying ConnectionId?


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:

http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@696
PS4, Line 696:
There's a mix of ASSERT and EXPECT below; curious why you aren't just using one 
and not the other?


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@697
PS4, Line 697:   // Verify the initial counters.
Test num_{client,server}_connections here too?


http://gerrit.cloudera.org:8080/#/c/11681/4/src/kudu/rpc/rpc-test.cc@698
PS4, Line 698:   ReactorMetrics metrics;
 :   
ASSERT_OK(server_messenger_->reactors_[0]->GetMetrics());
 :   ASSERT_EQ(0, metrics.total_client_connections_);
 :   ASSERT_EQ(0, metrics.total_server_connections_);
 :   
ASSERT_OK(client_messenger->reactors_[0]->GetMetrics());
 :   ASSERT_EQ(0, metrics.total_client_connections_);
 :   ASSERT_EQ(0, metrics.total_server_connections_);
This is repeated below; perhaps define a 2-arg lambda to encapsulate the whole 
thing?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6767e631fd9530ea54f5ed63ff4c8c179ab216b2
Gerrit-Change-Number: 11681
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 07 Nov 2018 00:06:18 +
Gerrit-HasComments: Yes


[kudu-CR] KUDU-2584: Prevent flaky off-by-one errors in backup tests

2018-11-06 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11815 )

Change subject: KUDU-2584: Prevent flaky off-by-one errors in backup tests
..


Patch Set 3: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11815/2/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
File 
java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala:

http://gerrit.cloudera.org:8080/#/c/11815/2/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@364
PS2, Line 364: nough that data is i
> No, I just wanted to be correct. I can remove it.
Yeah I would remove it. It's important for this explanation to be clear, so 
anything not germane should be elided.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0f1b4a4138cc8c913543a68fad748927cdc439d
Gerrit-Change-Number: 11815
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Tue, 06 Nov 2018 22:00:34 +
Gerrit-HasComments: Yes


[kudu-CR] Limit number of rowsets in compaction selection to 32 in TSAN mode

2018-11-06 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11885 )

Change subject: Limit number of rowsets in compaction selection to 32 in TSAN 
mode
..


Patch Set 2: Code-Review+2

> Patch Set 2:
> I'm not sure what typical usage per column would be, though. Is it a lot less 
> than 256KB?

Where did that 256KB figure come from? Can you point me at it?


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I01ad4ba3a13995c194c3308d72c1eb9b611ef766
Gerrit-Change-Number: 11885
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Tue, 06 Nov 2018 21:58:33 +
Gerrit-HasComments: No


[kudu-CR] KUDU-2584: Prevent flaky off-by-one errors in backup tests

2018-11-05 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11815 )

Change subject: KUDU-2584: Prevent flaky off-by-one errors in backup tests
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/11815/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11815/2//COMMIT_MSG@13
PS2, Line 13: The underlying reason for adding 1 ms is that we pass
: the timestamp in ms granularity but the snapshot time
: consists of microseconds plus a logical clock. This
: means if the data is inserted with a fraction of a ms
: remaining it could be truncated and unread.
I left some comments on this in the code; please also update this paragraph 
accordingly.


http://gerrit.cloudera.org:8080/#/c/11815/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala:

http://gerrit.cloudera.org:8080/#/c/11815/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@138
PS2, Line 138: 
kuduContext.timestampAccumulator.add(kuduContext.syncClient.getLastPropagatedTimestamp)
This is so that if you're using the same KuduContext for a backup job and a 
write, the timestamps "seen" during the backup job's scan will propagate into 
the clients so that they'll be sent during the write too?


http://gerrit.cloudera.org:8080/#/c/11815/2/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala
File 
java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala:

http://gerrit.cloudera.org:8080/#/c/11815/2/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@364
PS2, Line 364: plus a logical clock
This detail isn't relevant to the problem/solution discussion, is it?


http://gerrit.cloudera.org:8080/#/c/11815/2/java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala@364
PS2, Line 364: This means
 : // if the data is inserted with a fraction of a ms remaining 
it could be truncated and
 : // unread.
In practice this is always true; what are the odds that a timestamp value of 
micro granularity is generated with exactly 0 micros?

So I think the more interesting bit to document is that the test has to run 
fast enough such that the currentTimeMillis() call on L353 ends up with the 
same ms value as the write's timestamp (after truncating the micros).



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia0f1b4a4138cc8c913543a68fad748927cdc439d
Gerrit-Change-Number: 11815
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Mon, 05 Nov 2018 23:48:48 +
Gerrit-HasComments: Yes


[kudu-CR] move HashAlgorithm from common.proto to hash.proto

2018-11-02 Thread Adar Dembo (Code Review)
Adar Dembo has removed a vote on this change.

Change subject: move HashAlgorithm from common.proto to hash.proto
..


Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11865
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I2b0a4077335de75dfed6aa1c6d78d9e197250976
Gerrit-Change-Number: 11865
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)


[kudu-CR] move HashAlgorithm from common.proto to hash.proto

2018-11-02 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11865 )

Change subject: move HashAlgorithm from common.proto to hash.proto
..


Patch Set 2: Verified+1

Overriding Jenkins, dist-test failure KUDU-2432.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2b0a4077335de75dfed6aa1c6d78d9e197250976
Gerrit-Change-Number: 11865
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 02 Nov 2018 21:06:44 +
Gerrit-HasComments: No


[kudu-CR] move HashAlgorithm from common.proto to hash.proto

2018-11-02 Thread Adar Dembo (Code Review)
Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/11865 )

Change subject: move HashAlgorithm from common.proto to hash.proto
..

move HashAlgorithm from common.proto to hash.proto

Commit 8af288a26 added a dependency from the bloom filter code onto common,
creating a circular dependency. Normally that's caught by cmake, but the
commit didn't actually establish the util->common dependency in cmake, which
meant that the underlying parallelism of the build (and whether it was a
clean or incremental build) dictated whether common.pb.h was present on-disk
at the time that bloom_filter.h was included.

This patch resolves the circular dependency by moving the HashAlgorithm enum
out of common.proto and into a new hash.proto within util. An alternative
would be to move the bloom filter code itself into common, but as it isn't
specific to Kudu, I think util is a better home for it.

Change-Id: I2b0a4077335de75dfed6aa1c6d78d9e197250976
Reviewed-on: http://gerrit.cloudera.org:8080/11865
Reviewed-by: Grant Henke 
Reviewed-by: Andrew Wong 
Tested-by: Adar Dembo 
---
M src/kudu/common/CMakeLists.txt
M src/kudu/common/common.proto
M src/kudu/util/CMakeLists.txt
M src/kudu/util/bloom_filter.h
A src/kudu/util/hash.proto
5 files changed, 55 insertions(+), 19 deletions(-)

Approvals:
  Grant Henke: Looks good to me, approved
  Andrew Wong: Looks good to me, approved
  Adar Dembo: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2b0a4077335de75dfed6aa1c6d78d9e197250976
Gerrit-Change-Number: 11865
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)


[kudu-CR] move HashAlgorithm from common.proto to hash.proto

2018-11-02 Thread Adar Dembo (Code Review)
Hello Kudu Jenkins, Andrew Wong, Grant Henke,

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

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

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

Change subject: move HashAlgorithm from common.proto to hash.proto
..

move HashAlgorithm from common.proto to hash.proto

Commit 8af288a26 added a dependency from the bloom filter code onto common,
creating a circular dependency. Normally that's caught by cmake, but the
commit didn't actually establish the util->common dependency in cmake, which
meant that the underlying parallelism of the build (and whether it was a
clean or incremental build) dictated whether common.pb.h was present on-disk
at the time that bloom_filter.h was included.

This patch resolves the circular dependency by moving the HashAlgorithm enum
out of common.proto and into a new hash.proto within util. An alternative
would be to move the bloom filter code itself into common, but as it isn't
specific to Kudu, I think util is a better home for it.

Change-Id: I2b0a4077335de75dfed6aa1c6d78d9e197250976
---
M src/kudu/common/CMakeLists.txt
M src/kudu/common/common.proto
M src/kudu/util/CMakeLists.txt
M src/kudu/util/bloom_filter.h
A src/kudu/util/hash.proto
5 files changed, 55 insertions(+), 19 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2b0a4077335de75dfed6aa1c6d78d9e197250976
Gerrit-Change-Number: 11865
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)


[kudu-CR] move HashAlgorithm from common.proto to hash.proto

2018-11-02 Thread Adar Dembo (Code Review)
Hello Andrew Wong, Grant Henke,

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

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

to review the following change.


Change subject: move HashAlgorithm from common.proto to hash.proto
..

move HashAlgorithm from common.proto to hash.proto

Commit 8af288a26 added a dependency from the bloom filter code onto common,
creating a circular dependency. Normally that's caught by cmake, but the
commit didn't actually establish the util->common dependency in cmake, which
meant that the underlying parallelism of the build (and whether it was a
clean or incremental build) dictated whether common.pb.h was present on-disk
at the time that bloom_filter.h was included.

This patch resolves the circular dependency by moving the HashAlgorithm enum
out of common.proto and into a new hash.proto within util. An alternative
would be to move the bloom filter code itself into common, but as it isn't
specific to Kudu, I think util is a better home for it.

Change-Id: I2b0a4077335de75dfed6aa1c6d78d9e197250976
---
M src/kudu/common/CMakeLists.txt
M src/kudu/common/common.proto
M src/kudu/util/CMakeLists.txt
M src/kudu/util/bloom_filter.h
A src/kudu/util/hash.proto
5 files changed, 55 insertions(+), 18 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2b0a4077335de75dfed6aa1c6d78d9e197250976
Gerrit-Change-Number: 11865
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Grant Henke 


[kudu-CR] [test] optional table creation in TabletServerIntegrationTestBase

2018-11-02 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11861 )

Change subject: [test] optional table creation in 
TabletServerIntegrationTestBase
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I818a2eaf76fdee11ae792c27eddae311b775efeb
Gerrit-Change-Number: 11861
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 02 Nov 2018 18:35:55 +
Gerrit-HasComments: No


[kudu-CR] (05/05) delta applier: support for diff scan style iteration

2018-11-02 Thread Adar Dembo (Code Review)
Hello Mike Percy, David Ribeiro Alves, Kudu Jenkins, Grant Henke, Todd Lipcon,

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

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

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

Change subject: (05/05) delta_applier: support for diff scan style iteration
..

(05/05) delta_applier: support for diff scan style iteration

This patch incorporates all of the previous work to provide diff scan
support for an entire DiskRowSet. There's a new "fuzzy" test in
diskrowset-test that exercises this functionality.

Change-Id: I895350a3c7728df88a87a36b904ffdea4f3f6b7a
---
M src/kudu/tablet/delta_applier.cc
M src/kudu/tablet/delta_applier.h
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/diskrowset-test.cc
5 files changed, 286 insertions(+), 9 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I895350a3c7728df88a87a36b904ffdea4f3f6b7a
Gerrit-Change-Number: 11860
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] (04/05) delta store: support iteration with is deleted virtual column

2018-11-02 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11859 )

Change subject: (04/05) delta_store: support iteration with is_deleted virtual 
column
..


Patch Set 2: Verified+1

RELEASE build hung in a Python test. I posted 
http://gerrit.cloudera.org:8080/11863 to address that.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibafcae9f9f82a6c2d9a8afedb8fbf07d6e3069ec
Gerrit-Change-Number: 11859
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Nov 2018 16:10:30 +
Gerrit-HasComments: No


[kudu-CR] python: make test scantoken more robust to exceptions

2018-11-02 Thread Adar Dembo (Code Review)
Hello Jordan Birdsell, Andrew Wong,

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

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

to review the following change.


Change subject: python: make test_scantoken more robust to exceptions
..

python: make test_scantoken more robust to exceptions

One of my precommit tests failed with:

  22:49:05 kudu/tests/test_client.py .
  22:49:06 kudu/tests/test_scanner.py 
  22:49:08 kudu/tests/test_scantoken.py ..Build timed out (after 50 minutes). 
Marking the build as failed.
  23:39:08 Build was aborted

I don't know why this happened, but looking at commit 1d928951e, it seems
like if pool.map() raised an exception, we wouldn't close or join the pool,
which would lead to the same test hanging behavior as before. I don't know
how or why pool.map() raised an exception, but this will hopefully surface
the actual root cause.

Change-Id: I9b5a70d6b5f115cc4de740ded6ecc48574b9c707
---
M python/kudu/tests/test_scantoken.py
1 file changed, 5 insertions(+), 3 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9b5a70d6b5f115cc4de740ded6ecc48574b9c707
Gerrit-Change-Number: 11863
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Jordan Birdsell 


[kudu-CR] (04/05) delta store: support iteration with is deleted virtual column

2018-11-02 Thread Adar Dembo (Code Review)
Adar Dembo has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/11859 )

Change subject: (04/05) delta_store: support iteration with is_deleted virtual 
column
..


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11859
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ibafcae9f9f82a6c2d9a8afedb8fbf07d6e3069ec
Gerrit-Change-Number: 11859
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] (02/05) delta store: convert DeltaIterator::PrepareBatch flags into bitfield

2018-11-02 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11857 )

Change subject: (02/05) delta_store: convert DeltaIterator::PrepareBatch flags 
into bitfield
..


Patch Set 2: Verified+1

Overriding Jenkins, flaky TSAN test failure in DefaultSourceTest.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id500ec3cc9459a78ae1098be3f3cd0cacb7a7a1a
Gerrit-Change-Number: 11857
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 02 Nov 2018 15:50:01 +
Gerrit-HasComments: No


[kudu-CR] (02/05) delta store: convert DeltaIterator::PrepareBatch flags into bitfield

2018-11-02 Thread Adar Dembo (Code Review)
Adar Dembo has removed Kudu Jenkins from this change.  ( 
http://gerrit.cloudera.org:8080/11857 )

Change subject: (02/05) delta_store: convert DeltaIterator::PrepareBatch flags 
into bitfield
..


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11857
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Id500ec3cc9459a78ae1098be3f3cd0cacb7a7a1a
Gerrit-Change-Number: 11857
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] [test] optional table creation in TabletServerIntegrationTestBase

2018-11-02 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11861 )

Change subject: [test] optional table creation in 
TabletServerIntegrationTestBase
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11861/1/src/kudu/integration-tests/ts_itest-base.cc
File src/kudu/integration-tests/ts_itest-base.cc:

http://gerrit.cloudera.org:8080/#/c/11861/1/src/kudu/integration-tests/ts_itest-base.cc@550
PS1, Line 550:   if (create_table) {
 : NO_FATALS(CreateTable());
 :   }
 :   if (create_table) {
 : WaitForTSAndReplicas();
 : ASSERT_FALSE(tablet_replicas_.empty());
 : tablet_id_ = (*tablet_replicas_.begin()).first;
 :   } else {
 : WaitForTabletServers();
 :   }
Combine?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I818a2eaf76fdee11ae792c27eddae311b775efeb
Gerrit-Change-Number: 11861
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Fri, 02 Nov 2018 15:49:18 +
Gerrit-HasComments: Yes


[kudu-CR] (02/05) delta store: convert DeltaIterator::PrepareBatch flags into bitfield

2018-11-01 Thread Adar Dembo (Code Review)
Hello Mike Percy, David Ribeiro Alves, Kudu Jenkins, Grant Henke, Todd Lipcon,

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

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

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

Change subject: (02/05) delta_store: convert DeltaIterator::PrepareBatch flags 
into bitfield
..

(02/05) delta_store: convert DeltaIterator::PrepareBatch flags into bitfield

A follow-on patch will introduce DeltaIterator::SelectUpdates(), a new
method for extracting delta information from a DeltaIterator. As it is
only to be used by diff scans and will require dedicated in-memory state,
it'll also come with a new DeltaIterator::PrepareBatch() flag.

However, diff scans will need to use both SelectUpdates() and
ApplyUpdates(), and it'd be a shame to have to reseek and reprepare just to
use both extraction methods. As such, this patch makes it possible to
prepare a batch for use in multiple delta extraction methods.

Change-Id: Id500ec3cc9459a78ae1098be3f3cd0cacb7a7a1a
---
M src/kudu/tablet/delta_iterator_merger.cc
M src/kudu/tablet/delta_iterator_merger.h
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
M src/kudu/tablet/tablet-test-util.h
9 files changed, 55 insertions(+), 80 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id500ec3cc9459a78ae1098be3f3cd0cacb7a7a1a
Gerrit-Change-Number: 11857
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] (01/05) delta store: avoid copying deleted row data in ApplyUpdates

2018-11-01 Thread Adar Dembo (Code Review)
Hello Mike Percy, David Ribeiro Alves, Kudu Jenkins, Grant Henke, Todd Lipcon,

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

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

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

Change subject: (01/05) delta_store: avoid copying deleted row data in 
ApplyUpdates
..

(01/05) delta_store: avoid copying deleted row data in ApplyUpdates

When applying deltas, the scan path will first populate a selection vector
with deletes (i.e. a row is unset if there's a relevant DELETE for it), and
then use it to enable two optimizations:
1. Short-circuit out if all rows in the block have been deleted.
2. Skip predicate evaluation and base data copying for deleted rows.

To this we can add a third optimization: don't apply a delta whose mutation
is to a deleted row. Note that it's not incorrect to apply such deltas (as
we'll skip deleted rows when serializing the scan response to the client);
it's just unnecessary work.

I wrote a microbenchmark to evaluate the impact of this optimization. Below
is the timing and perf stat output of the microbenchmark before and after
applying the optimization (specifically, applying this patch, then
commenting out the filtering in DeltaPrepare::ApplyUpdates for the first
run, and uncommenting it for the next run).

  Time spent running 1000 scans with 0 deletes: real 0.523s user 0.524s 
sys 0.000s
  Time spent running 1000 scans with 10 deletes: real 0.515suser 0.516s 
sys 0.000s
  Time spent running 1000 scans with 100 deletes: real 0.512s   user 0.512s 
sys 0.000s
  Time spent running 1000 scans with 1000 deletes: real 0.553s  user 0.552s 
sys 0.000s

   Performance counter stats for 'bin/deltamemstore-test 
--gtest_filter=*Varying* --benchmark_num_passes=1000':

   2201.029290  task-clock (msec) #0.991 CPUs utilized
 5  context-switches  #0.002 K/sec
 0  cpu-migrations#0.000 K/sec
 4,950  page-faults   #0.002 M/sec
 8,276,723,832  cycles#3.760 GHz
24,539,935,941  instructions  #2.96  insn per cycle
 4,709,709,705  branches  # 2139.776 M/sec
12,631,579  branch-misses #0.27% of all branches

   2.220370506 seconds time elapsed

  Time spent running 1000 scans with 0 deletes: real 0.474s user 0.475s 
sys 0.000s
  Time spent running 1000 scans with 10 deletes: real 0.475suser 0.472s 
sys 0.004s
  Time spent running 1000 scans with 100 deletes: real 0.478s   user 0.476s 
sys 0.004s
  Time spent running 1000 scans with 1000 deletes: real 0.550s  user 0.552s 
sys 0.000s

   Performance counter stats for 'bin/deltamemstore-test 
--gtest_filter=*Varying* --benchmark_num_passes=1000':

   2074.795741  task-clock (msec) #0.990 CPUs utilized
23  context-switches  #0.011 K/sec
 1  cpu-migrations#0.000 K/sec
 4,951  page-faults   #0.002 M/sec
 7,675,100,058  cycles#3.699 GHz
23,100,692,252  instructions  #3.01  insn per cycle
 4,539,777,117  branches  # 2188.060 M/sec
11,819,267  branch-misses #0.26% of all branches

   2.096193851 seconds time elapsed

Note: I originally wrote this patch thinking it was necessary for diff scan
correctness, but have since convinced myself that it's just an optimization.

Change-Id: I6a646d0816a96e9aba486c0d0a1e6b4a7e15c144
---
M src/kudu/tablet/delta_applier.cc
M src/kudu/tablet/delta_applier.h
M src/kudu/tablet/delta_iterator_merger.cc
M src/kudu/tablet/delta_iterator_merger.h
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
M src/kudu/tablet/tablet-test-util.h
13 files changed, 116 insertions(+), 46 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6a646d0816a96e9aba486c0d0a1e6b4a7e15c144
Gerrit-Change-Number: 11856
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] (05/05) delta applier: support for diff scan style iteration

2018-11-01 Thread Adar Dembo (Code Review)
Hello Mike Percy, David Ribeiro Alves, Kudu Jenkins, Grant Henke, Todd Lipcon,

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

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

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

Change subject: (05/05) delta_applier: support for diff scan style iteration
..

(05/05) delta_applier: support for diff scan style iteration

This patch incorporates all of the previous work to provide diff scan
support for an entire DiskRowSet. There's a new "fuzzy" test in
diskrowset-test that exercises this functionality.

Change-Id: I895350a3c7728df88a87a36b904ffdea4f3f6b7a
---
M src/kudu/tablet/delta_applier.cc
M src/kudu/tablet/delta_applier.h
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/diskrowset-test.cc
5 files changed, 287 insertions(+), 9 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I895350a3c7728df88a87a36b904ffdea4f3f6b7a
Gerrit-Change-Number: 11860
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] (03/05) delta store: support iteration with snap to exclude

2018-11-01 Thread Adar Dembo (Code Review)
Hello Mike Percy, David Ribeiro Alves, Kudu Jenkins, Grant Henke, Todd Lipcon,

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

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

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

Change subject: (03/05) delta_store: support iteration with snap_to_exclude
..

(03/05) delta_store: support iteration with snap_to_exclude

This patch changes the delta stores (DMS and delta files) to respect
snap_to_exclude during iteration. The key changes are:
- The introduction of the "selection" criteria, a new delta relevancy
  formula for determining whether a delta applies to a scan with both
  snap_to_exclude and snap_to_include. The existing "application" criteria
  was formalized and moved into delta_relevancy.h. There was also a
  non-trivial change to DeltaFileReader::IsRelevantForSnapshot() to use both
  criterias when culling entire delta files.
- A new SelectUpdates() method for using the selection criteria on a batch
  of prepared deltas. SelectUpdates() requires new in-memory state, the
  creation of which is gated behind a new PREPARE_FOR_SELECT flag so as not
  to affect regular scans.
- Updates to the delta fuzz testing logic to test iterators with two
  timestamps, and to provide SelectUpdates() coverage.

A future patch will modify DeltaApplier to use SelectUpdates for diff scans.

Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c
---
M src/kudu/tablet/delta_iterator_merger.cc
M src/kudu/tablet/delta_iterator_merger.h
A src/kudu/tablet/delta_relevancy.h
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
M src/kudu/tablet/tablet-test-util.cc
M src/kudu/tablet/tablet-test-util.h
11 files changed, 468 insertions(+), 92 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c
Gerrit-Change-Number: 11858
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] (04/05) delta store: support iteration with is deleted virtual column

2018-11-01 Thread Adar Dembo (Code Review)
Hello Mike Percy, David Ribeiro Alves, Kudu Jenkins, Grant Henke, Todd Lipcon,

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

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

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

Change subject: (04/05) delta_store: support iteration with is_deleted virtual 
column
..

(04/05) delta_store: support iteration with is_deleted virtual column

This patch adds support to delta stores (i.e. DMS and delta files) for
iterating with an IS_DELETED virtual column. A diff scan will include
IS_DELETED in the projection so that clients can tell if a row was deleted
in the requested time range.

Change-Id: Ibafcae9f9f82a6c2d9a8afedb8fbf07d6e3069ec
---
M src/kudu/common/schema.h
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/tablet-test-util.h
7 files changed, 99 insertions(+), 26 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibafcae9f9f82a6c2d9a8afedb8fbf07d6e3069ec
Gerrit-Change-Number: 11859
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] deltamemstore: support iteration with snap to exclude

2018-11-01 Thread Adar Dembo (Code Review)
Adar Dembo has abandoned this change. ( http://gerrit.cloudera.org:8080/11029 )

Change subject: deltamemstore: support iteration with snap_to_exclude
..


Abandoned

This ended up being more complicated and I wrote a new series of changes to do 
it.
--
To view, visit http://gerrit.cloudera.org:8080/11029
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I04af3737960f3fcc7b1921a77ff91e1607b7bc47
Gerrit-Change-Number: 11029
Gerrit-PatchSet: 11
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] deltas: add SelectUpdates iterator method

2018-11-01 Thread Adar Dembo (Code Review)
Adar Dembo has abandoned this change. ( http://gerrit.cloudera.org:8080/11137 )

Change subject: deltas: add SelectUpdates iterator method
..


Abandoned

Ended up merging this into another change, because it made more sense to do so.
--
To view, visit http://gerrit.cloudera.org:8080/11137
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I5dda1787b8dfa64bc86800b8883499929eef9fef
Gerrit-Change-Number: 11137
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] WIP: DeltaApplier changes

2018-11-01 Thread Adar Dembo (Code Review)
Adar Dembo has abandoned this change. ( http://gerrit.cloudera.org:8080/11139 )

Change subject: WIP: DeltaApplier changes
..


Abandoned

Revamped this in a new change.
--
To view, visit http://gerrit.cloudera.org:8080/11139
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I787932ef49d2207e00a4e30a6cced9a9c0f7eb89
Gerrit-Change-Number: 11139
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)


[kudu-CR] WIP: DeltaFileIterator snap to exclude

2018-11-01 Thread Adar Dembo (Code Review)
Adar Dembo has abandoned this change. ( http://gerrit.cloudera.org:8080/11138 )

Change subject: WIP: DeltaFileIterator snap_to_exclude
..


Abandoned

Revamped this in a new change.
--
To view, visit http://gerrit.cloudera.org:8080/11138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I5fa5e4c689d3d8a5a56f8bdd91d2047e1f2f85a6
Gerrit-Change-Number: 11138
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)


[kudu-CR] delta store: support iteration with snap to exclude

2018-11-01 Thread Adar Dembo (Code Review)
Hello Mike Percy, David Ribeiro Alves, Grant Henke, Todd Lipcon,

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

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

to review the following change.


Change subject: delta_store: support iteration with snap_to_exclude
..

delta_store: support iteration with snap_to_exclude

This patch changes the delta stores (DMS and delta files) to respect
snap_to_exclude during iteration. The key changes are:
- The introduction of the "selection" criteria, a new delta relevancy
  formula for determining whether a delta applies to a scan with both
  snap_to_exclude and snap_to_include. The existing "application" criteria
  was formalized and moved into delta_relevancy.h. There was also a
  non-trivial change to DeltaFileReader::IsRelevantForSnapshot() to use both
  criterias when culling entire delta files.
- A new SelectUpdates() method for using the selection criteria on a batch
  of prepared deltas. SelectUpdates() requires new in-memory state, the
  creation of which is gated behind a new PREPARE_FOR_SELECT flag so as not
  to affect regular scans.
- Updates to the delta fuzz testing logic to test iterators with two
  timestamps, and to provide SelectUpdates() coverage.

A future patch will modify DeltaApplier to use SelectUpdates for diff scans.

Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c
---
M src/kudu/tablet/delta_iterator_merger.cc
M src/kudu/tablet/delta_iterator_merger.h
A src/kudu/tablet/delta_relevancy.h
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
M src/kudu/tablet/tablet-test-util.cc
M src/kudu/tablet/tablet-test-util.h
11 files changed, 468 insertions(+), 92 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7811e185fef270f40fdbbb38f491eee8b4aa043c
Gerrit-Change-Number: 11858
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] delta store: convert DeltaIterator::PrepareBatch flags into bitfield

2018-11-01 Thread Adar Dembo (Code Review)
Hello Mike Percy, David Ribeiro Alves, Grant Henke, Todd Lipcon,

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

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

to review the following change.


Change subject: delta_store: convert DeltaIterator::PrepareBatch flags into 
bitfield
..

delta_store: convert DeltaIterator::PrepareBatch flags into bitfield

A follow-on patch will introduce DeltaIterator::SelectUpdates(), a new
method for extracting delta information from a DeltaIterator. As it is
only to be used by diff scans and will require dedicated in-memory state,
it'll also come with a new DeltaIterator::PrepareBatch() flag.

However, diff scans will need to use both SelectUpdates() and
ApplyUpdates(), and it'd be a shame to have to reseek and reprepare just to
use both extraction methods. As such, this patch makes it possible to
prepare a batch for use in multiple delta extraction methods.

Change-Id: Id500ec3cc9459a78ae1098be3f3cd0cacb7a7a1a
---
M src/kudu/tablet/delta_iterator_merger.cc
M src/kudu/tablet/delta_iterator_merger.h
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
M src/kudu/tablet/tablet-test-util.h
9 files changed, 55 insertions(+), 80 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id500ec3cc9459a78ae1098be3f3cd0cacb7a7a1a
Gerrit-Change-Number: 11857
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] delta store: avoid copying deleted row data in ApplyUpdates

2018-11-01 Thread Adar Dembo (Code Review)
Hello Mike Percy, David Ribeiro Alves, Grant Henke, Todd Lipcon,

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

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

to review the following change.


Change subject: delta_store: avoid copying deleted row data in ApplyUpdates
..

delta_store: avoid copying deleted row data in ApplyUpdates

When applying deltas, the scan path will first populate a selection vector
with deletes (i.e. a row is unset if there's a relevant DELETE for it), and
then use it to enable two optimizations:
1. Short-circuit out if all rows in the block have been deleted.
2. Skip predicate evaluation and base data copying for deleted rows.

To this we can add a third optimization: don't apply a delta whose mutation
is to a deleted row. Note that it's not incorrect to apply such deltas (as
we'll skip deleted rows when serializing the scan response to the client);
it's just unnecessary work.

I wrote a microbenchmark to evaluate the impact of this optimization. Below
is the timing and perf stat output of the microbenchmark before and after
applying the optimization (specifically, applying this patch, then
commenting out the filtering in DeltaPrepare::ApplyUpdates for the first
run, and uncommenting it for the next run).

  Time spent running 1000 scans with 0 deletes: real 0.523s user 0.524s 
sys 0.000s
  Time spent running 1000 scans with 10 deletes: real 0.515suser 0.516s 
sys 0.000s
  Time spent running 1000 scans with 100 deletes: real 0.512s   user 0.512s 
sys 0.000s
  Time spent running 1000 scans with 1000 deletes: real 0.553s  user 0.552s 
sys 0.000s

   Performance counter stats for 'bin/deltamemstore-test 
--gtest_filter=*Varying* --benchmark_num_passes=1000':

   2201.029290  task-clock (msec) #0.991 CPUs utilized
 5  context-switches  #0.002 K/sec
 0  cpu-migrations#0.000 K/sec
 4,950  page-faults   #0.002 M/sec
 8,276,723,832  cycles#3.760 GHz
24,539,935,941  instructions  #2.96  insn per cycle
 4,709,709,705  branches  # 2139.776 M/sec
12,631,579  branch-misses #0.27% of all branches

   2.220370506 seconds time elapsed

  Time spent running 1000 scans with 0 deletes: real 0.474s user 0.475s 
sys 0.000s
  Time spent running 1000 scans with 10 deletes: real 0.475suser 0.472s 
sys 0.004s
  Time spent running 1000 scans with 100 deletes: real 0.478s   user 0.476s 
sys 0.004s
  Time spent running 1000 scans with 1000 deletes: real 0.550s  user 0.552s 
sys 0.000s

   Performance counter stats for 'bin/deltamemstore-test 
--gtest_filter=*Varying* --benchmark_num_passes=1000':

   2074.795741  task-clock (msec) #0.990 CPUs utilized
23  context-switches  #0.011 K/sec
 1  cpu-migrations#0.000 K/sec
 4,951  page-faults   #0.002 M/sec
 7,675,100,058  cycles#3.699 GHz
23,100,692,252  instructions  #3.01  insn per cycle
 4,539,777,117  branches  # 2188.060 M/sec
11,819,267  branch-misses #0.26% of all branches

   2.096193851 seconds time elapsed

Note: I originally wrote this patch thinking it was necessary for diff scan
correctness, but have since convinced myself that it's just an optimization.

Change-Id: I6a646d0816a96e9aba486c0d0a1e6b4a7e15c144
---
M src/kudu/tablet/delta_applier.cc
M src/kudu/tablet/delta_applier.h
M src/kudu/tablet/delta_iterator_merger.cc
M src/kudu/tablet/delta_iterator_merger.h
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
M src/kudu/tablet/tablet-test-util.h
13 files changed, 116 insertions(+), 46 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6a646d0816a96e9aba486c0d0a1e6b4a7e15c144
Gerrit-Change-Number: 11856
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] delta store: support iteration with is deleted virtual column

2018-11-01 Thread Adar Dembo (Code Review)
Hello Mike Percy, David Ribeiro Alves, Grant Henke, Todd Lipcon,

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

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

to review the following change.


Change subject: delta_store: support iteration with is_deleted virtual column
..

delta_store: support iteration with is_deleted virtual column

This patch adds support to delta stores (i.e. DMS and delta files) for
iterating with an IS_DELETED virtual column. A diff scan will include
IS_DELETED in the projection so that clients can tell if a row was deleted
in the requested time range.

Change-Id: Ibafcae9f9f82a6c2d9a8afedb8fbf07d6e3069ec
---
M src/kudu/common/schema.h
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/tablet-test-util.h
7 files changed, 99 insertions(+), 26 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibafcae9f9f82a6c2d9a8afedb8fbf07d6e3069ec
Gerrit-Change-Number: 11859
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] delta applier: support for diff scan style iteration

2018-11-01 Thread Adar Dembo (Code Review)
Hello Mike Percy, David Ribeiro Alves, Grant Henke, Todd Lipcon,

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

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

to review the following change.


Change subject: delta_applier: support for diff scan style iteration
..

delta_applier: support for diff scan style iteration

This patch incorporates all of the previous work to provide diff scan
support for an entire DiskRowSet. There's a new "fuzzy" test in
diskrowset-test that exercises this functionality.

Change-Id: I895350a3c7728df88a87a36b904ffdea4f3f6b7a
---
M src/kudu/tablet/delta_applier.cc
M src/kudu/tablet/delta_applier.h
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/diskrowset-test.cc
5 files changed, 287 insertions(+), 9 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I895350a3c7728df88a87a36b904ffdea4f3f6b7a
Gerrit-Change-Number: 11860
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR](refs/meta/config) Change e-mail notification recipient to reviews@kudu.apache.org

2018-11-01 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11840 )

Change subject: Change e-mail notification recipient to reviews@kudu.apache.org
..


Patch Set 3:

Oops, I pushed to refs/meta/config instead of to refs/for/meta/config, so I 
wound up merging the change instead of publishing a new revision.

Sorry about that.


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

Gerrit-Project: kudu
Gerrit-Branch: refs/meta/config
Gerrit-MessageType: comment
Gerrit-Change-Id: I45f3a070108eb71931f85c317294cbc90b9bc521
Gerrit-Change-Number: 11840
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 01 Nov 2018 17:43:57 +
Gerrit-HasComments: No


[kudu-CR](refs/meta/config) Change e-mail notification recipient to reviews@kudu.apache.org

2018-11-01 Thread Adar Dembo (Code Review)
Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/11840 )

Change subject: Change e-mail notification recipient to reviews@kudu.apache.org
..

Change e-mail notification recipient to reviews@kudu.apache.org

We were still using revi...@kudu.incubator.apache.org for some reason.

Change-Id: I45f3a070108eb71931f85c317294cbc90b9bc521
---
M project.config
1 file changed, 12 insertions(+), 12 deletions(-)

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

Gerrit-Project: kudu
Gerrit-Branch: refs/meta/config
Gerrit-MessageType: merged
Gerrit-Change-Id: I45f3a070108eb71931f85c317294cbc90b9bc521
Gerrit-Change-Number: 11840
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR](refs/meta/config) Change e-mail notification recipient to reviews@kudu.apache.org

2018-11-01 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11840 )

Change subject: Change e-mail notification recipient to reviews@kudu.apache.org
..


Patch Set 3: Verified+1 Code-Review+2

Carrying forward Andrew's +2.


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

Gerrit-Project: kudu
Gerrit-Branch: refs/meta/config
Gerrit-MessageType: comment
Gerrit-Change-Id: I45f3a070108eb71931f85c317294cbc90b9bc521
Gerrit-Change-Number: 11840
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 01 Nov 2018 17:43:09 +
Gerrit-HasComments: No


[kudu-CR](refs/meta/config) Change e-mail notification recipient to reviews@kudu.apache.org

2018-11-01 Thread Adar Dembo (Code Review)
Hello Andrew Wong, Todd Lipcon,

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

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

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

Change subject: Change e-mail notification recipient to reviews@kudu.apache.org
..

Change e-mail notification recipient to reviews@kudu.apache.org

We were still using revi...@kudu.incubator.apache.org for some reason.

Change-Id: I45f3a070108eb71931f85c317294cbc90b9bc521
---
M project.config
1 file changed, 12 insertions(+), 12 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: refs/meta/config
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I45f3a070108eb71931f85c317294cbc90b9bc521
Gerrit-Change-Number: 11840
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Todd Lipcon 


[kudu-CR] [util] Add ParseStringsWithScheme in net util

2018-11-01 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11843 )

Change subject: [util] Add ParseStringsWithScheme in net_util
..


Patch Set 3: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc
File src/kudu/util/net/net_util.cc:

http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@153
PS1, Line 153:
> Hmm, I did not see how since StripPrefixString or StripSuffixString are rel
Ah, yeah I saw what you mean; since the entirety of the scheme and path aren't 
known up front, you can't use those functions to find them.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0
Gerrit-Change-Number: 11843
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 01 Nov 2018 17:13:20 +
Gerrit-HasComments: Yes


[kudu-CR] [sentry] add AuthzProvider

2018-11-01 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11659 )

Change subject: [sentry] add AuthzProvider
..


Patch Set 7: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 17:13:55 +
Gerrit-HasComments: No


[kudu-CR] [sentry] add AuthzProvider

2018-10-31 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11659 )

Change subject: [sentry] add AuthzProvider
..


Patch Set 6: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772
Gerrit-Change-Number: 11659
Gerrit-PatchSet: 6
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 01 Nov 2018 04:17:22 +
Gerrit-HasComments: No


[kudu-CR] [util] Add ParseStringsWithScheme in net util

2018-10-31 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11843 )

Change subject: [util] Add ParseStringsWithScheme in net_util
..


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h
File src/kudu/util/net/net_util.h:

http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h@46
PS1, Line 46:   // Similar to above but allow the address to be have scheme and 
path. e.g.
Should mention that the scheme and path are ignored.


http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.h@76
PS1, Line 76:   // Similar to above but allow the addresses to be have scheme 
and path.
Same.


http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc
File src/kudu/util/net/net_util.cc:

http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@149
PS1, Line 149: Status HostPort::ParseStringWithScheme(const std::string , 
uint16_t default_port) {
No std:: prefix. Also const string&


http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@150
PS1, Line 150:   string uri(str);
Nit: maybe name this "str_copy" so it's more clear that it's a copy?


http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@153
PS1, Line 153:
Maybe you can simplify some of this with gutil functions like 
StripPrefixString, StripSuffixString, etc?


http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@217
PS1, Line 217:   vector addr_strings = strings::Split(comma_sep_addrs, 
",", strings::SkipEmpty());
Once you've got this, you can reserve the appropriate capacity in res.


http://gerrit.cloudera.org:8080/#/c/11843/1/src/kudu/util/net/net_util.cc@221
PS1, Line 221: res->push_back(host_port);
Nit: emplace_back for new code.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2447ab80e0b0737c4eb2ba8216769a52b5c07ce0
Gerrit-Change-Number: 11843
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 01 Nov 2018 04:14:17 +
Gerrit-HasComments: Yes


[kudu-CR] KUDU-686 (part 1/2): decompose guts of DMSIterator into DeltaPreparer

2018-10-31 Thread Adar Dembo (Code Review)
Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11394 )

Change subject: KUDU-686 (part 1/2): decompose guts of DMSIterator into 
DeltaPreparer
..


Patch Set 12: Code-Review+2

Carrying forward David's +2.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26c92dfa69b5323e7fbb18d02010ed99cc29c3e5
Gerrit-Change-Number: 11394
Gerrit-PatchSet: 12
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 31 Oct 2018 23:58:07 +
Gerrit-HasComments: No


[kudu-CR] KUDU-686 (part 2/2): use DeltaPreparer in DeltaFileIterator

2018-10-31 Thread Adar Dembo (Code Review)
Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/11395 )

Change subject: KUDU-686 (part 2/2): use DeltaPreparer in DeltaFileIterator
..

KUDU-686 (part 2/2): use DeltaPreparer in DeltaFileIterator

This patch rewrites much of DeltaFileIterator to leverage DeltaPreparer.
Besides sharing code, the motivation is to take advantage of the performance
improvement inherent to DeltaPreparer: decoding a batch of deltas just once
in PrepareBatch() as opposed to on each call to ApplyUpdates().

Seeing as DeltaPreparer was originally built for DMSIterator, here are the
various augmentations that were necessary to support DeltaFileIterator:
- REINSERT support, which meant more complicated deletion state tracking.
- FilterColumnIdsAndCollectDeltas support, cribbed from DeltaFileIterator.
- A templatized traits system to control which features were enabled. This
  also meant templatizing both DeltaPreparer and DeltaFileIterator.
- Early out from the "apply all deltas for a row" loop when the timestamp
  is no longer relevant. There's an opportunity to seek here and skip any
  remaining deltas belonging to the row, but testing with a new DMS iterator
  microbenchmark showed that this is only an improvement when the number of
  deltas skipped is sufficiently high.

The improvement should be most noticeable on tables with wide schemas where
multiple columns are projected. In these situations, the column-by-column
ApplyUpdates() approach incurred a lot of unnecessary delta decoding. To
prove it, I included a new deltafile iterator microbenchmark that scans a
subset of a wide schema's columns as a DeltaApplier would.

Before:

  Performance counter stats for 'bin/deltafile-test --gtest_filter=*Benchmark*' 
(5 runs):

  11358.256100  task-clock (msec) #0.998 CPUs utilized  
  ( +-  3.39% )
   140  context-switches  #0.012 K/sec  
  ( +- 27.37% )
 6  cpu-migrations#0.001 K/sec  
  ( +- 52.36% )
34,231  page-faults   #0.003 M/sec  
  ( +-  0.42% )
42,288,292,153  cycles#3.723 GHz
  ( +-  4.12% )
   100,853,942,114  instructions  #2.38  insn per cycle 
  ( +-  5.35% )
19,689,789,259  branches  # 1733.522 M/sec  
  ( +-  5.49% )
69,419,478  branch-misses #0.35% of all branches
  ( +-  5.14% )

  11.378958537 seconds time elapsed 
 ( +-  3.38% )

After:

  Performance counter stats for 'bin/deltafile-test --gtest_filter=*Benchmark*' 
(5 runs):

   4089.419224  task-clock (msec) #0.995 CPUs utilized  
  ( +-  1.25% )
43  context-switches  #0.011 K/sec  
  ( +-  4.10% )
 2  cpu-migrations#0.000 K/sec  
  ( +- 32.39% )
34,948  page-faults   #0.009 M/sec  
  ( +-  0.22% )
15,269,907,971  cycles#3.734 GHz
  ( +-  1.30% )
32,409,048,370  instructions  #2.12  insn per cycle 
  ( +-  1.85% )
 5,848,268,795  branches  # 1430.098 M/sec  
  ( +-  1.85% )
32,900,262  branch-misses #0.56% of all branches
  ( +-  2.80% )

   4.111096224 seconds time elapsed 
 ( +-  1.18% )

It's interesting to see the number of page faults increase while everything
else has gone down, but that makes sense: the new implementation allocates
memory in PrepareBatch() in order to optimize the structure of the deltas.

To be extra safe, I also looped fuzz-itest 1000 times in slow mode. All of
the runs passed.

Change-Id: I87de52092262c4b42c1bd5107f9139edfc3888b5
Reviewed-on: http://gerrit.cloudera.org:8080/11395
Tested-by: Adar Dembo 
Reviewed-by: Adar Dembo 
---
M src/kudu/tablet/delta_key.h
M src/kudu/tablet/delta_store.cc
M src/kudu/tablet/delta_store.h
M src/kudu/tablet/deltafile-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/deltamemstore-test.cc
M src/kudu/tablet/deltamemstore.cc
M src/kudu/tablet/deltamemstore.h
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet-test-util.h
12 files changed, 525 insertions(+), 403 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved; Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I87de52092262c4b42c1bd5107f9139edfc3888b5

  1   2   3   4   5   6   7   8   9   10   >