[kudu-CR] Bump test timeout for flex partitioning-itest

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: Bump test timeout for flex_partitioning-itest
..


Bump test timeout for flex_partitioning-itest

In precommit runs, this test is sharded 8 ways so it never has issues with
timeouts. However, in some Jenkins builds that don't use dist-test,
it can time out, especially when TSAN is enabled.

This adds functionality to our ADD_KUDU_TEST() cmake function to specify
a timeout and plumbs that timeout through to our test wrapper shell
script.

Tested by verifying that the appropriate environment variable gets set
in the environment of the running test.

Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Reviewed-on: http://gerrit.cloudera.org:8080/7814
Reviewed-by: Adar Dembo 
Tested-by: Todd Lipcon 
---
M CMakeLists.txt
M build-support/run-test.sh
M src/kudu/integration-tests/CMakeLists.txt
3 files changed, 46 insertions(+), 4 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Bump test timeout for flex partitioning-itest

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Bump test timeout for flex_partitioning-itest
..


Patch Set 3: Verified+1

Release build hit KUDU-2089 (java test retry left tmp files, causing the build 
to fail)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [java] Reuse JVM across tests

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [java] Reuse JVM across tests
..


Patch Set 1:

> Having the Java tests run in dist-test would be really nice. We can't 
> parallelize the unit tests locally because they have collisions on the 
> MiniCluster usage.

yea, that could be solved the same way that the C++ client solves it, though 
(having the minicluster daemons bind to 127.a.b.c rather than 127.0.0.1). 
Doesn't work on OSX but c'est la vie

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e4c6d53054d5da4f2ecf9f4130a51df13d4fa6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [java] Reuse JVM across tests

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change.

Change subject: [java] Reuse JVM across tests
..


Patch Set 1:

Having the Java tests run in dist-test would be really nice. We can't 
parallelize the unit tests locally because they have collisions on the 
MiniCluster usage.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e4c6d53054d5da4f2ecf9f4130a51df13d4fa6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [java] Reuse JVM across tests

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change.

Change subject: [java] Reuse JVM across tests
..


Patch Set 1:

Yeah, I agree with "guaranteed isolation" being more reliable. This patch came 
as a result of the gradle patch where weird issues were seen because I didn't 
use a different JVM per test. 

We can leave 1 JVM per test class, but these teardown fixes are still valid.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e4c6d53054d5da4f2ecf9f4130a51df13d4fa6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [java] Reuse JVM across tests

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change.

Change subject: [java] Reuse JVM across tests
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7825/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java:

Line 70:   public void testLowTimeoutScans() throws Exception {
I broke these tests out because it looks like sometimes a warm client could 
actually succeed in 1ms. Its also nice to have a method for each thing tested.


http://gerrit.cloudera.org:8080/#/c/7825/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
File 
java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala:

Line 350:   def clear(): Unit = {
Having cached connection that outlived the client was causing issues when we 
started a whole new cluster with a new certificates in later tests. Is their 
any real-world issue having JVM wide cached connections like this could cause?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e4c6d53054d5da4f2ecf9f4130a51df13d4fa6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java] Reuse JVM across tests

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [java] Reuse JVM across tests
..


Patch Set 1:

Not sure how I feel about this change. In my experience these kinds of things 
usually cause more hard-to-diagnose bugs than they are worth -- eg random tests 
end up leaving threads running in failure scenarios, and those threads end up 
logging stuff captured by future tests, interfering with them, etc.

If we want to improve test runtime I think we're better off focusing on 
distributed execution or parallel execution across many VMs, but still having a 
one-vm-per-test.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e4c6d53054d5da4f2ecf9f4130a51df13d4fa6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [java] Reuse JVM across tests

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [java] Reuse JVM across tests
..


Patch Set 1:

(1 comment)

Just passing through with a question.

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

Line 7: [java] Reuse JVM across tests
Could you measure the change in test running time and note it here?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e4c6d53054d5da4f2ecf9f4130a51df13d4fa6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] catalog manager: don't log deleted tables/tablets at startup

[Adar Dembo (Code Review)]

Hello Dan Burkert, Alexey Serbin,

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

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

to review the following change.

Change subject: catalog_manager: don't log deleted tables/tablets at startup
..

catalog_manager: don't log deleted tables/tablets at startup

On long-lived clusters this list can get quite long.

While I was in the area, I made some other cosmetic improvements:
- Applying strings::Substitute() to perforated LOG statements.
- Replacing 'OVERRIDE' with 'override'.
- Removing 'virtual' from overriden methods.
- Removing unnecessary std:: prefixes.
- A few other miscellaneous tweaks.

Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/sys_catalog.h
2 files changed, 191 insertions(+), 205 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] [java] Reuse JVM across tests

[Grant Henke (Code Review)]

Grant Henke has uploaded a new change for review.

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

Change subject: [java] Reuse JVM across tests
..

[java] Reuse JVM across tests

Currently each java test is run in a separate JVM.This is extra overhead and 
can cause the tests torun longer. This patch ensures “shared” resources 
arecleaned up properly across tests and reconfigures thetest runs to reuse the 
JVM.

Change-Id: I8e4c6d53054d5da4f2ecf9f4130a51df13d4fa6c
---
M java/gradle/tests.gradle
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M 
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
M java/pom.xml
7 files changed, 50 insertions(+), 21 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8e4c6d53054d5da4f2ecf9f4130a51df13d4fa6c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 

[kudu-CR] java-client: improve error messages when failing to connect to secure cluster

[Dan Burkert (Code Review)]

Hello Jean-Daniel Cryans, Alexey Serbin,

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

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

to review the following change.

Change subject: java-client: improve error messages when failing to connect to 
secure cluster
..

java-client: improve error messages when failing to connect to secure cluster

Cleans up the error handling in Connection.java so that is passes
through non-recoverable exceptions, and improves the error which results
from attempting to connect to a secure cluster with an unauthenticate
client. Example of the new error:

org.apache.kudu.client.NonRecoverableException:
  Couldn't find a valid master in (nightly512-1.gce.cloudera.com:7051).
  Exceptions received:
  org.apache.kudu.client.NonRecoverableException:
  Server requires Kerberos, but this client is not authenticated 
(kinit)

Change-Id: I41c3229dcda284dce57cb6f6930efe3b50fa9698
---
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java
2 files changed, 66 insertions(+), 51 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I41c3229dcda284dce57cb6f6930efe3b50fa9698
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..


KUDU-1943: Add BlockTransaction to Block Manager

This adds a new layer of abstraction 'BlockTransaction' to
Block Manager, to accumulate new blocks in a logical operation,
e.g. flush/compaction. Avoid using fsync() per block can
potentionaly reduce a lot I/O overhead.

This patch also add a new API, Finalize(), to Block Manager,
so that an LBM container can be reused without flushing
in-flight writable blocks to disk. This can bring down the
log block container number and disk space consumption for
first few flush/compaction operations when using log block
manager.

I performed manual test which creates a table with two tablets,
'kudu perf loadgen localhost --num-rows-per-thread=2000
 --num-threads=10 --keep-auto-table --table_num_buckets=2'.
With this patch, 'log_block_manager_containers' dropped from 56
to 7.

A design doc can be found here:
https://docs.google.com/document/d/1cB8pDs-Ho7p2HlzkWo1d-wmXr90YntIjdvdOv7u7gn0/edit?usp=sharing

Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Reviewed-on: http://gerrit.cloudera.org:8080/7207
Reviewed-by: Adar Dembo 
Tested-by: Adar Dembo 
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tablet/multi_column_writer.h
20 files changed, 615 insertions(+), 453 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved; Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 35
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..


Patch Set 34: Verified+1

We can't let IWYU get in the way of progress.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 34
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] Fix flaky ts recovery-itest TestChangeMaxCellSize

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: Fix flaky ts_recovery-itest TestChangeMaxCellSize
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7820/1/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

Line 1014:   if (!replica->error().ok()) {
> This has an ABA problem, potentially. How about:
Hrm, sure. Setting error _should_ be permanent (in that it shouldn't go back to 
OK), but it's a reasonable safeguard against further state-transience changes.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f0a6b19756777e5f4081ef6c8cb5af4ecc8a3d6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Fix flaky ts recovery-itest TestChangeMaxCellSize

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Fix flaky ts_recovery-itest TestChangeMaxCellSize
..

Fix flaky ts_recovery-itest TestChangeMaxCellSize

In TSTabletManager::CreateReportedTabletPB(), each replica's state is
reported, and if this state is FAILED, it additionally reports the error
state that caused it to fail. However, replica state is transient, and
can, on rare occasion, change between these two retrievals of replica
state.

This led to a DCHECK failure in ts_recovery-itest when parsing a
report, as reports with errors are expected to be FAILED. The failure
was consistent with the following sequence of events:
  1. The tablet is opened, hits an error, and starts shutting down
  2. The report notes the replica state (STOPPING)
  3. The tablet finishes shutting down and switches its state (FAILED)
  4. The report, seeing the replica is FAILED, notes the error that
 caused the shut down
  5. The catalog manager goes through the report, and notes the error
 tacked onto the report and the fact that the tablet is STOPPING
 instead of FAILED. This fails the DCHECK, as logged below:

BlockManagerType/TsRecoveryITest.TestChangeMaxCellSize/0: 
catalog_manager.cc:2489] Check failed: report.state() == tablet::FAILED (3 vs. 
2)

This sequence may be more frequent since a9d17c0, which enforces the
tablet stops (i.e. go into the STOPPING state and shut down the
replica's internals) before going into the FAILED state, instead of
going directly to the FAILED state.

Validated by injecting a pause in the CreateReportedTabletPB between the
two calls and hitting the DCHECK failure. This is fixed by reporting the
error regardless of state and removing this DCHECK.

Change-Id: I6f0a6b19756777e5f4081ef6c8cb5af4ecc8a3d6
---
M src/kudu/master/catalog_manager.cc
M src/kudu/tserver/ts_tablet_manager.cc
2 files changed, 4 insertions(+), 5 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6f0a6b19756777e5f4081ef6c8cb5af4ecc8a3d6
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy
..


Patch Set 7: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] Fix flaky ts recovery-itest TestChangeMaxCellSize

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Fix flaky ts_recovery-itest TestChangeMaxCellSize
..


Patch Set 1:

(1 comment)

Yeah, that DCHECK is not useful.

http://gerrit.cloudera.org:8080/#/c/7820/1/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

Line 1014:   if (!replica->error().ok()) {
This has an ABA problem, potentially. How about:

  const Status& error = replica->error();
  if (!error.ok()) {
StatusToPB(reported_tablet->mutable_error(), error_status);
  }


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6f0a6b19756777e5f4081ef6c8cb5af4ecc8a3d6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [tests] fix flakiness in catalog manager tsk-itest

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: [tests] fix flakiness in catalog_manager_tsk-itest
..

[tests] fix flakiness in catalog_manager_tsk-itest

After recent updates the catalog_manager_tsk-itest became unstable
in TSAN configuration.  The failure scenario looks like the following:

  * 3 masters are running with --raft_enable_pre_election=false flag
and short raft heartbeat interval.

  * The test client sends CreateTable request. The server side completes
the request, but once the storm of master re-elections starts,
the client cannot get the authoritative answer on its
IsCreateTableDone requests because master leadership changes
every 40-100ms.  As as result, after retrying IsCreateTableDone
request many times, the client times out on its CreateTable request.

Making the raft heartbeat interval higher helps keeping the master
leadership intervals longer, so the client has better chance of
getting authoritative response from the masters.

Change-Id: I3731fb560923d82d2fb64ac7b829080f99a0681e
---
M src/kudu/integration-tests/catalog_manager_tsk-itest.cc
1 file changed, 1 insertion(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I3731fb560923d82d2fb64ac7b829080f99a0681e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 

[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7701/6//COMMIT_MSG
Commit Message:

Line 17: has its own dedicated single disk.
> And just to be clear, each tablet server was using its own, dedicated disk,
Right, added the commit message accordingly.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy

[Hao Hao (Code Review)]

Hello Mike Percy, Kudu Jenkins,

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

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

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

Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy
..

KUDU-1726: Avoid fsync-per-block in tablet copy

This patch incorporates BlockTransaction API into tablet
copy, to avoid fsync() on each block during copying. Blocks
are synced to disk together once the tablet copy is complete.
It also introduces a new block manager metric 'total_disk_sync'
to keep track of block data disk synchronization count.

I did a manual test to copy tablet with size of ~37GB from one
tablet server to another on el6 with ext4. Each tablet server
has its own dedicated single disk.
'kudu remote_replica copy c53ffbc2ede84b6d9af2da607024d131
localhost:3334 localhost:3335'

With this change, the operation time dropped down from ~718s
to ~523s.

Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76
---
M src/kudu/fs/block_manager_metrics.cc
M src/kudu/fs/block_manager_metrics.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
7 files changed, 71 insertions(+), 17 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/7701/7
-- 
To view, visit http://gerrit.cloudera.org:8080/7701
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..


Patch Set 34:

Not sure why IWYU is complaining about including ext/alloc_traits.h for 
log_block_manager.cc, etc?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 34
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..


Patch Set 34: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 34
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Hao Hao (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..

KUDU-1943: Add BlockTransaction to Block Manager

This adds a new layer of abstraction 'BlockTransaction' to
Block Manager, to accumulate new blocks in a logical operation,
e.g. flush/compaction. Avoid using fsync() per block can
potentionaly reduce a lot I/O overhead.

This patch also add a new API, Finalize(), to Block Manager,
so that an LBM container can be reused without flushing
in-flight writable blocks to disk. This can bring down the
log block container number and disk space consumption for
first few flush/compaction operations when using log block
manager.

I performed manual test which creates a table with two tablets,
'kudu perf loadgen localhost --num-rows-per-thread=2000
 --num-threads=10 --keep-auto-table --table_num_buckets=2'.
With this patch, 'log_block_manager_containers' dropped from 56
to 7.

A design doc can be found here:
https://docs.google.com/document/d/1cB8pDs-Ho7p2HlzkWo1d-wmXr90YntIjdvdOv7u7gn0/edit?usp=sharing

Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tablet/multi_column_writer.h
20 files changed, 615 insertions(+), 453 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/34
-- 
To view, visit http://gerrit.cloudera.org:8080/7207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 34
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] gradle: target JRE 7 compatibility

[Dan Burkert (Code Review)]

Dan Burkert has submitted this change and it was merged.

Change subject: gradle: target JRE 7 compatibility
..


gradle: target JRE 7 compatibility

Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29
Reviewed-on: http://gerrit.cloudera.org:8080/7785
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
Reviewed-by: Grant Henke 
Reviewed-by: Adar Dembo 
---
M java/gradle.properties
M java/gradle/compile.gradle
2 files changed, 7 insertions(+), 5 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Grant Henke: Looks good to me, but someone else must approve
  Todd Lipcon: Looks good to me, but someone else must approve
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] gradle: target JRE 7 compatibility

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: gradle: target JRE 7 compatibility
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7821/1/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:

Line 131: // Hard code the SSL_OP_NO_TLSv1 and SSL_OP_NO_TLSv1_1 flags from 
OpenSSL
> Makes sense, but rather than the macros here, can we do something like this
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions

[Dan Burkert (Code Review)]

Dan Burkert has uploaded a new patch set (#2).

Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
..

rpc: allow setting --rpc_tls_min_protocol on older RHEL versions

Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
---
M src/kudu/security/tls_context.cc
1 file changed, 25 insertions(+), 12 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7821/1/src/kudu/security/tls_context.cc
File src/kudu/security/tls_context.cc:

Line 131: // Hard code the SSL_OP_NO_TLSv1 and SSL_OP_NO_TLSv1_1 flags from 
OpenSSL
Makes sense, but rather than the macros here, can we do something like this 
earlier, after including the openssl headers:

  #ifndef SSL_OP_NO_TLSv1
  #define SSL_OP_NO_TLSv1 0x0400U
  #endif

  #ifndef SSL_OP_NO_TLSv1_1
  #define SSL_OP_NO_TLSv1_1 0x1000U
  #endif

That should improve readability in the code down here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions

[Dan Burkert (Code Review)]

Hello Henry Robinson, Todd Lipcon,

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

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

to review the following change.

Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions
..

rpc: allow setting --rpc_tls_min_protocol on older RHEL versions

Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
---
M src/kudu/security/tls_context.cc
1 file changed, 20 insertions(+), 6 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add more trace counters and timings

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Add more trace counters and timings
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7819/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

Line 764:   virtual Status Read(uint64_t offset, Slice* result) const OVERRIDE {
> I think this already gets traced at the LBM level, no?
Yeah but you don't like having traces at multiple levels? What if we have heavy 
non-LBM RWFile usage in the future?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie99bc9e06be682e1595745f154b7dded3903bd6e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add more trace counters and timings

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Add more trace counters and timings
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7819/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

Line 764:   virtual Status Read(uint64_t offset, Slice* result) const OVERRIDE {
> No tracing for reads/writes?
I think this already gets traced at the LBM level, no?


Line 786: TRACE_EVENT1("io", "PosixRWFile::PreAllocate", "path", filename_);
> Want to include the offset/length?
this is just moved from below, but sure.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie99bc9e06be682e1595745f154b7dded3903bd6e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Bump test timeout for flex partitioning-itest

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Bump test timeout for flex_partitioning-itest
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add more trace counters and timings

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Add more trace counters and timings
..


Patch Set 1:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/7819/1/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

PS1, Line 1245:   int64_t dur = end_time - start_time;
  :   TRACE_COUNTER_INCREMENT("lbm_write_time_us", dur);
  :   const char* counter = BUCKETED_COUNTER_NAME("lbm_writes", 
dur);
  :   TRACE_COUNTER_INCREMENT(counter, 1);
Maybe move this into the container too, for symmetry?


PS1, Line 1458:   int64_t dur = end_time - start_time;
  :   TRACE_COUNTER_INCREMENT("lbm_read_time_us", dur);
  : 
  :   const char* counter = BUCKETED_COUNTER_NAME("lbm_reads", dur);
  :   TRACE_COUNTER_INCREMENT(counter, 1);
And this.


http://gerrit.cloudera.org:8080/#/c/7819/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

Line 764:   virtual Status Read(uint64_t offset, Slice* result) const OVERRIDE {
No tracing for reads/writes?


Line 786: TRACE_EVENT1("io", "PosixRWFile::PreAllocate", "path", filename_);
Want to include the offset/length?


Line 827: TRACE_EVENT1("io", "PosixRWFile::PunchHole", "path", filename_);
Could include offset/length here too.


Line 863: TRACE_EVENT1("io", "PosixRWFile::Flush", "path", filename_);
And here too.


Line 902: TRACE_EVENT1("io", "PosixRWFile::Close", "path", filename_);
Trace close_us and number of close operations?


Line 1280: TRACE_EVENT1("io", "PosixEnv::GetFileSizeOnDisk", "path", fname);
Trace micros and number of operations?

Same for the other stat-based functions below.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie99bc9e06be682e1595745f154b7dded3903bd6e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] Fix flaky ts recovery-itest TestChangeMaxCellSize

[Andrew Wong (Code Review)]

Andrew Wong has uploaded a new change for review.

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

Change subject: Fix flaky ts_recovery-itest TestChangeMaxCellSize
..

Fix flaky ts_recovery-itest TestChangeMaxCellSize

In TSTabletManager::CreateReportedTabletPB(), each replica's state is
reported, and if this state is FAILED, it additionally reports the error
state that caused it to fail. However, replica state is transient, and
can, on rare occasion, change between these two retrievals of replica
state.

This led to a DCHECK failure in ts_recovery-itest when parsing a
report, as reports with errors are expected to be FAILED. The failure
was consistent with the following sequence of events:
  1. The tablet is opened, hits an error, and starts shutting down
  2. The report notes the replica state (STOPPING)
  3. The tablet finishes shutting down and switches its state (FAILED)
  4. The report, seeing the replica is FAILED, notes the error that
 caused the shut down
  5. The catalog manager goes through the report, and notes the error
 tacked onto the report and the fact that the tablet is STOPPING
 instead of FAILED. This fails the DCHECK, as logged below:

BlockManagerType/TsRecoveryITest.TestChangeMaxCellSize/0: 
catalog_manager.cc:2489] Check failed: report.state() == tablet::FAILED (3 vs. 
2)

This sequence may be more frequent since a9d17c0, which enforces the
tablet stop (i.e. go into the STOPPING state and shut down the replica's
internals) before going into the FAILED state, instead of going directly
to the FAILED state.

This was validated by injecting a pause in the CreateReportedTabletPB
between the two calls and hitting the DCHECK failure. This is fixed by
reporting the error regardless of state and removing this DCHECK.

Change-Id: I6f0a6b19756777e5f4081ef6c8cb5af4ecc8a3d6
---
M src/kudu/master/catalog_manager.cc
M src/kudu/tserver/ts_tablet_manager.cc
2 files changed, 2 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6f0a6b19756777e5f4081ef6c8cb5af4ecc8a3d6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 

[kudu-CR] Bump test timeout for flex partitioning-itest

[Todd Lipcon (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: Bump test timeout for flex_partitioning-itest
..

Bump test timeout for flex_partitioning-itest

In precommit runs, this test is sharded 8 ways so it never has issues with
timeouts. However, in some Jenkins builds that don't use dist-test,
it can time out, especially when TSAN is enabled.

This adds functionality to our ADD_KUDU_TEST() cmake function to specify
a timeout and plumbs that timeout through to our test wrapper shell
script.

Tested by verifying that the appropriate environment variable gets set
in the environment of the running test.

Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
---
M CMakeLists.txt
M build-support/run-test.sh
M src/kudu/integration-tests/CMakeLists.txt
3 files changed, 46 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Bump test timeout for flex partitioning-itest

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Bump test timeout for flex_partitioning-itest
..


Patch Set 2:

(1 comment)

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

Line 673: set(ARG_TIMEOUT 900)
> Maybe a comment about how this should be kept in-sync with the default valu
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add more trace counters and timings

[Todd Lipcon (Code Review)]

Hello Adar Dembo,

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

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

to review the following change.

Change subject: Add more trace counters and timings
..

Add more trace counters and timings

This afternoon I was looking at some DeleteTablet calls that were taking longer
than expected. The trace metrics were rather incomplete, so it wasn't clear
where the time was coming from. This adds some more tracing to various
code paths related to IO and tablet metadata.

Change-Id: Ie99bc9e06be682e1595745f154b7dded3903bd6e
---
M src/kudu/fs/log_block_manager.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/util/env_posix.cc
3 files changed, 65 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie99bc9e06be682e1595745f154b7dded3903bd6e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 

[kudu-CR] Bump test timeout for flex partitioning-itest

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Bump test timeout for flex_partitioning-itest
..


Patch Set 2:

(1 comment)

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

Line 673: set(ARG_TIMEOUT 900)
Maybe a comment about how this should be kept in-sync with the default value in 
run-test.sh, and a comment there too?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7701/6//COMMIT_MSG
Commit Message:

Line 17: has a single disk.
And just to be clear, each tablet server was using its own, dedicated disk, 
right? The two weren't sharing the same disk?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] [security] avoid kerberos ticket renewal and only reacquire

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: [security] avoid kerberos ticket renewal and only reacquire
..


Patch Set 1:

> I think there is mention of 'renew' in init.h. Also I think we can
 > remove the renew_lifetime handling in mini_kdc.{h,cc} as well as
 > security-faults-itest.cc.

I removed the mention of renew in init.* and other relevant places. However, I 
think it will be beneficial to leave in the renew_lifetime in the mini_kdc just 
to ensure that it works under different configurations, if and when we choose 
to add more tests that use the minikdc in the future.

For eg: Add more tests to configure kerberos differently to attempt to catch 
issues like KUDU-2032.

What do you think?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e5225de332ba785e3a73014b8418cfd4059fe07
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Bump test timeout for flex partitioning-itest

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Bump test timeout for flex_partitioning-itest
..

Bump test timeout for flex_partitioning-itest

In precommit runs, this test is sharded 8 ways so it never has issues with
timeouts. However, in some Jenkins builds that don't use dist-test,
it can time out, especially when TSAN is enabled.

This adds functionality to our ADD_KUDU_TEST() cmake function to specify
a timeout and plumbs that timeout through to our test wrapper shell
script.

Tested by verifying that the appropriate environment variable gets set
in the environment of the running test.

Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
---
M CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
2 files changed, 40 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Bump test timeout for flex partitioning-itest

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Bump test timeout for flex_partitioning-itest
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7814/1/CMakeLists.txt
File CMakeLists.txt:

Line 673: set(ARG_TIMEOUT 900)
> Can we avoid defining this default value both here and in run-test.sh? Give
Yea, I considered doing this (not passing any TIMEOUT if there is nothing set). 
However I thought it was nice to have the "belt and suspenders" timeout 
structure here -- if for some reason the KuduTest timeout mechanism gets stuck, 
it's nice to have ctest applying a timeout as well.

This isn't totally academic - we've seen cases where the 'pstack' which comes 
when a test timeout hits some bug in gdb which makes it hang, and then the 
entire build fails. So having ctest kill the test 30 seconds later would 
prevent that.


Line 707:   # Set the ctest timeout to be a bit longer than the timeout we pass 
to
> Why bother with the ctest timeout at all, given that the run-test.sh timeou
yea, see above


Line 714:   if("${CUR_TEST_ENV}" STREQUAL "NOTFOUND")
> does if(NOT CUR_TEST_ENV) work?
ah, I think it might, will try that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy
..


Patch Set 6:

(1 comment)

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

PS1, Line 13: to keep track of block data disk synchronization count.
: 
: I did a m
> What about the disk setup? How many disks did each tserver have? Did they s
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy
..


Patch Set 6:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7701/5/src/kudu/fs/file_block_manager.cc
File src/kudu/fs/file_block_manager.cc:

PS5, Line 390:  
> See comment in log_block_manager.cc regarding this metric, and below
I think this one is in the right place, only updated for SyncMetadata below.


http://gerrit.cloudera.org:8080/#/c/7701/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 886:   RETURN_NOT_OK(SyncData());
> hmm. Should these metric increments happen inside SyncData() so they remain
Makes sense. Updated.


Line 1870:   MakeContainerAvailableUnlocked(container);
> Should also track this, no?
Done


http://gerrit.cloudera.org:8080/#/c/7701/5/src/kudu/tserver/tablet_copy_client-test.cc
File src/kudu/tserver/tablet_copy_client-test.cc:

Line 91: metric_entity_ = 
METRIC_ENTITY_server.Instantiate(_registry_, "test");
> I'm surprised it's OK for the registry to go out of scope and be destroyed 
Right... Thanks for catching it.


http://gerrit.cloudera.org:8080/#/c/7701/5/src/kudu/tserver/tablet_copy_client.h
File src/kudu/tserver/tablet_copy_client.h:

PS5, Line 178: Data block is opened with new ID. 
> I don't understand what this remnant is trying to convey.
Yeah, actually I do not quite follow the original comment. But let me try to 
rephrase it based on my understanding.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy

[Hao Hao (Code Review)]

Hello Mike Percy, Kudu Jenkins,

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

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

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

Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy
..

KUDU-1726: Avoid fsync-per-block in tablet copy

This patch incorporates BlockTransaction API into tablet
copy, to avoid fsync() on each block during copying. Blocks
are synced to disk together once the tablet copy is complete.
It also introduces a new block manager metric 'total_disk_sync'
to keep track of block data disk synchronization count.

I did a manual test to copy tablet with size of ~37GB from one
tablet server to another on el6 with ext4. Each tablet server
has a single disk.
'kudu remote_replica copy c53ffbc2ede84b6d9af2da607024d131
localhost:3334 localhost:3335'

With this change, the operation time dropped down from ~718s
to ~523s.

Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76
---
M src/kudu/fs/block_manager_metrics.cc
M src/kudu/fs/block_manager_metrics.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/tserver/tablet_copy_client-test.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_client.h
7 files changed, 68 insertions(+), 17 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] KUDU-2098. Drop Spark 1 Support

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change.

Change subject: KUDU-2098. Drop Spark 1 Support
..


Patch Set 1:

Yeah, I think that makes sense.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5481cc0477f4d23d89b68ef510a6c9a2aa187537
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] gradle: target JRE 7 compatibility

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change.

Change subject: gradle: target JRE 7 compatibility
..


Patch Set 2:

Would +2 if I could. :D

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] gradle: target JRE 7 compatibility

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change.

Change subject: gradle: target JRE 7 compatibility
..


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..


Patch Set 33: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 33
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Hao Hao (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..

KUDU-1943: Add BlockTransaction to Block Manager

This adds a new layer of abstraction 'BlockTransaction' to
Block Manager, to accumulate new blocks in a logical operation,
e.g. flush/compaction. Avoid using fsync() per block can
potentionaly reduce a lot I/O overhead.

This patch also add a new API, Finalize(), to Block Manager,
so that an LBM container can be reused without flushing
in-flight writable blocks to disk. This can bring down the
log block container number and disk space consumption for
first few flush/compaction operations when using log block
manager.

I performed manual test which creates a table with two tablets,
'kudu perf loadgen localhost --num-rows-per-thread=2000
 --num-threads=10 --keep-auto-table --table_num_buckets=2'.
With this patch, 'log_block_manager_containers' dropped from 56
to 7.

A design doc can be found here:
https://docs.google.com/document/d/1cB8pDs-Ho7p2HlzkWo1d-wmXr90YntIjdvdOv7u7gn0/edit?usp=sharing

Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tablet/multi_column_writer.h
20 files changed, 602 insertions(+), 437 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/33
-- 
To view, visit http://gerrit.cloudera.org:8080/7207
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 33
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..


Patch Set 32:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

Line 132
> There's no avoiding a downcast in LogBlockManager::CloseBlocks; we need it 
OK sounds good to me, it can stay as is.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 32
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] [security] avoid kerberos ticket renewal and only reacquire

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [security] avoid kerberos ticket renewal and only reacquire
..


Patch Set 1:

I think there is mention of 'renew' in init.h. Also I think we can remove the 
renew_lifetime handling in mini_kdc.{h,cc} as well as security-faults-itest.cc.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8e5225de332ba785e3a73014b8418cfd4059fe07
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [java] KUDU-2103 Canonicalize hostnames in client

[Attila Bukor (Code Review)]

Attila Bukor has posted comments on this change.

Change subject: [java] KUDU-2103 Canonicalize hostnames in client
..


Patch Set 6:

You're welcome, thanks for merging it in. I've backported it to 1.3.x and 
1.4.x, tested it locally using a Kerberized cluster, both versions worked fine.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Attila Bukor 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR](branch-1.3.x) [java] KUDU-2103 Canonicalize hostnames in client

[Attila Bukor (Code Review)]

Attila Bukor has uploaded a new change for review.

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

Change subject: [java] KUDU-2103 Canonicalize hostnames in client
..

[java] KUDU-2103 Canonicalize hostnames in client

When Kerberos is used the service principal contains the
hostname of the service. This means that if a user wants to use
an alias for the master_addresses in a Kerberized environment
it won't be able to connect as the service principal name is not
found in the Kerberos database.

This patch adds canonicalization for the hostnames to make sure
the principal name the service ticket is requested for matches
the one used by the master servers.

Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/FakeDNS.java
A java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
4 files changed, 124 insertions(+), 6 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Attila Bukor 

[kudu-CR] Fix flakiness of kudu-admin-test TestMoveTablet

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Fix flakiness of kudu-admin-test TestMoveTablet
..


Patch Set 2:

> Going to ignore IWYU since I dont think I touched anything that
 > should affect includes.

You touched the raft_consensus.cc file, which was not IWYU compliant due to 
prior updates.  I fixed that in f9a6a1b2d15feb8e2c044f20248774105a3a62bf.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] Fix flakiness of kudu-admin-test TestMoveTablet

[Todd Lipcon (Code Review)]

Hello Adar Dembo, Will Berkeley,

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

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

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

Change subject: Fix flakiness of kudu-admin-test TestMoveTablet
..

Fix flakiness of kudu-admin-test TestMoveTablet

This fixes an issue in the admin tool for moving a tablet that would occur if
the tablet changed terms multiple times before settling during an election.

Previously, we would wait for a term change, and as soon as we saw any term
change, we'd wait for an operation in exactly that term to be committed. There
were two issues with the code:

1) It used cstate.has_leader_uuid() to check if there was a leader in the new
term. However, the server side actually sets this field to an empty string
rather than leaving it unset if there is no leader. So this check always
evaluated to true, meaning that we would proceed to looking for a committed op
in that term as soon as the term advanced, rather than waiting for an actual
leader to be elected.

2) In the case that somehow the leader changed a second time, the term could
be increased again while we are waiting for a committed operation. In that case
we'd be waiting for a committed op in exactly the term of the first leader
change we saw, rather than potentially refreshing our notion of the "current
term".

The patch fixes both issues by restructuring the loop a bit.

I additionally improved some logging in the consensus service and
implementation which I found helpful while debugging the issue.

This test became significantly more flaky (~20% in ASAN) after the commit
of 21b0f3d5e255760535e281efe5879fe657df1f1c which adjusted the election
timeout behavior. Apparently the new election behavior made it more likely
for the elections to have conflicts before getting a steady leader, which
exposed the above bug in the tool.

With the patch, I was able to run 500/500 successful in an ASAN build (vs
20% fail rate before).

Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/tools/tool_action_tablet.cc
M src/kudu/tserver/tablet_service.cc
4 files changed, 54 insertions(+), 48 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] Fix flakiness of kudu-admin-test TestMoveTablet

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Fix flakiness of kudu-admin-test TestMoveTablet
..


Patch Set 1: Verified+1

Going to ignore IWYU since I dont think I touched anything that should affect 
includes.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] Bump test timeout for flex partitioning-itest

[Todd Lipcon (Code Review)]

Hello Adar Dembo,

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

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

to review the following change.

Change subject: Bump test timeout for flex_partitioning-itest
..

Bump test timeout for flex_partitioning-itest

In precommit runs, this test is sharded 8 ways so it never has issues with
timeouts. However, in some Jenkins builds that don't use dist-test,
it can time out, especially when TSAN is enabled.

This adds functionality to our ADD_KUDU_TEST() cmake function to specify
a timeout and plumbs that timeout through to our test wrapper shell
script.

Tested by verifying that the appropriate environment variable gets set
in the environment of the running test.

Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
---
M CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
2 files changed, 40 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 

[kudu-CR] [rpc] faster generation of KRPC call ID

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [rpc] faster generation of KRPC call ID
..


Patch Set 2:

(1 comment)

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

PS2, Line 48:   const int u_bound = 8 + (random() % 100);
:   int n = 0;
:   for (int i = 0; i < u_bound; ++i) {
: n = next_id();
:   }
> Yep, I suspected that it's not a good benchmark, but the newer code looked 
yea, if you compile in clang it turns it into a constant time operation. g++ on 
my machine isn't quite smart enough but it seems it is on yours, perhaps. 
Adding the noinline attribute on the function breaks that optimization of 
course and it goes back to being the same speed.

Either way, I think it's a "if it aint broke dont fix it" scenario IMO


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I03726343d222bcd241c2c2a5a1670a672f8e5cb6
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [rpc] faster generation of KRPC call ID

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [rpc] faster generation of KRPC call ID
..


Patch Set 2:

(1 comment)

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

PS2, Line 48:   const int u_bound = 8 + (random() % 100);
:   int n = 0;
:   for (int i = 0; i < u_bound; ++i) {
: n = next_id();
:   }
> I think your microbenchmark is invalid. With -O3 it compiles to this assemb
Yep, I suspected that it's not a good benchmark, but the newer code looked 
better IMO.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I03726343d222bcd241c2c2a5a1670a672f8e5cb6
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [rpc] faster generation of KRPC call ID

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [rpc] faster generation of KRPC call ID
..


Patch Set 2:

(1 comment)

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

PS2, Line 48:   const int u_bound = 8 + (random() % 100);
:   int n = 0;
:   for (int i = 0; i < u_bound; ++i) {
: n = next_id();
:   }
> I think your microbenchmark is invalid. With -O3 it compiles to this assemb
oh wait, I may have misread the assembly output... you can ignore this. But 
still I'm skeptical of the benchmark here.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I03726343d222bcd241c2c2a5a1670a672f8e5cb6
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [rpc] faster generation of KRPC call ID

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [rpc] faster generation of KRPC call ID
..


Patch Set 2: Code-Review-1

(1 comment)

don't think this is a worthwhile optimization since this isn't a hot code path

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

PS2, Line 48:   const int u_bound = 8 + (random() % 100);
:   int n = 0;
:   for (int i = 0; i < u_bound; ++i) {
: n = next_id();
:   }
I think your microbenchmark is invalid. With -O3 it compiles to this assembly:

int main() {
  4007a0:   48 83 ec 08 sub$0x8,%rsp
  srandom(time(nullptr));
  4007a4:   31 ff   xor%edi,%edi
  4007a6:   e8 d5 ff ff ff  callq  400780 
  4007ab:   89 c7   mov%eax,%edi
  4007ad:   e8 8e ff ff ff  callq  400740 
  const int u_bound = 8 + (random() % 100);
  4007b2:   e8 a9 ff ff ff  callq  400760 
  4007b7:   48 ba 0b d7 a3 70 3dmovabs $0xa3d70a3d70a3d70b,%rdx
  4007be:   0a d7 a3 
  4007c1:   48 89 c1mov%rax,%rcx
  4007c4:   48 f7 eaimul   %rdx
  4007c7:   48 89 c8mov%rcx,%rax
  4007ca:   48 c1 f8 3f sar$0x3f,%rax
  4007ce:   48 01 caadd%rcx,%rdx
  4007d1:   48 c1 fa 06 sar$0x6,%rdx
  4007d5:   48 29 c2sub%rax,%rdx
  4007d8:   48 8d 04 92 lea(%rdx,%rdx,4),%rax


ie it's turning the loop into a single constant time multiplication.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I03726343d222bcd241c2c2a5a1670a672f8e5cb6
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [rpc] faster generation of KRPC call ID

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new patch set (#2).

Change subject: [rpc] faster generation of KRPC call ID
..

[rpc] faster generation of KRPC call ID

Updated the way how the KRPC call identifiers are generated.

To compare, I used the following code (headers are omitted):

old.cc:
-

int32_t cur_id = 0;
int32_t next_id() {
  if (PREDICT_FALSE(cur_id == std::numeric_limits::max())) {
cur_id = 0;
  } else {
++cur_id;
  }
  return cur_id;
}

int main() {
  srandom(time(nullptr));
  const int u_bound = 8 + (random() % 100);
  int n = 0;
  for (int i = 0; i < u_bound; ++i) {
n = next_id();
  }
  cout << n << endl;
  return 0;
}
-

new.cc:
-
int32_t cur_id = 0;
int32_t next_id() {
  ++cur_id;
  return (cur_id & 0x7fff);
}

int main() {
  srandom(time(nullptr));
  const int u_bound = 8 + (random() % 100);
  int n = 0;
  for (int i = 0; i < u_bound; ++i) {
n = next_id();
  }
  std::cout << n << std::endl;
  return 0;
}
-

My ad-hoc measurements show that the new version is faster even if
compiling both with -O3 optimization flag:
  old:
real  0m0.831s
user  0m0.822s
sys   0m0.005s

  new:
real  0m0.005s
user  0m0.001s
sys   0m0.002s

>From the 'perf stat' perspective it looks good as well:

-
 Performance counter stats for './old' (10 runs):

790.584545 task-clock#1.001 CPUs utilized
 1 context-switches  #0.002 K/sec
 1 cpu-migrations#0.001 K/sec
   273 page-faults   #0.345 K/sec
 2,583,562,591 cycles#3.268 GHz
 2,805,308,074 instructions  #1.09  insns per cycle
   200,917,139 branches  #  254.137 M/sec
13,513 branch-misses #0.01% of all branches

   0.790023494 seconds time elapsed
-

-
 Performance counter stats for './new' (10 runs):

  0.888215 task-clock#0.831 CPUs utilized
 0 context-switches  #0.000 K/sec
 0 cpu-migrations#0.000 K/sec
   273 page-faults   #0.307 M/sec
 2,561,042 cycles#2.883 GHz
 2,804,468 instructions  #1.10  insns per cycle
   480,394 branches  #  540.853 M/sec
11,575 branch-misses #2.41% of all branches

   0.001068744 seconds time elapsed
-

Change-Id: I03726343d222bcd241c2c2a5a1670a672f8e5cb6
---
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
2 files changed, 9 insertions(+), 12 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I03726343d222bcd241c2c2a5a1670a672f8e5cb6
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [rpc] faster generation of KRPC call ID

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: [rpc] faster generation of KRPC call ID
..

[rpc] faster generation of KRPC call ID

Updated the way how the KRPC call identifiers are generated.

To compare, I used the following code (headers are omitted):

old.cc:
-

int32_t cur_id = 0;
int32_t next_id() {
  if (PREDICT_FALSE(cur_id == std::numeric_limits::max())) {
cur_id = 0;
  } else {
++cur_id;
  }
  return cur_id;
}

int main() {
  srandom(time(nullptr));
  const int u_bound = 8 + (random() % 100);
  int n = 0;
  for (int i = 0; i < u_bound; ++i) {
n = next_id();
  }
  cout << n << endl;
  return 0;
}
-

new.cc:
-
int32_t cur_id = 0;
int32_t next_id() {
  ++cur_id;
  return (cur_id & 0x7fff);
}

int main() {
  srandom(time(nullptr));
  const int u_bound = 8 + (random() % 100);
  int n = 0;
  for (int i = 0; i < u_bound; ++i) {
n = next_id();
  }
  std::cout << n << std::endl;
  return 0;
}
-

My ad-hoc measurements show that the new version is faster even if
compiling both with -O3 optimization flag:
  old:
real  0m0.831s
user  0m0.822s
sys   0m0.005s

  new:
real  0m0.005s
user  0m0.001s
sys   0m0.002s

>From the 'perf stat' perspective it looks good as well:

-
 Performance counter stats for './old' (10 runs):

790.584545 task-clock#1.001 CPUs utilized
 1 context-switches  #0.002 K/sec
 1 cpu-migrations#0.001 K/sec
   273 page-faults   #0.345 K/sec
 2,583,562,591 cycles#3.268 GHz
 2,805,308,074 instructions  #1.09  insns per cycle
   200,917,139 branches  #  254.137 M/sec
13,513 branch-misses #0.01% of all branches

   0.790023494 seconds time elapsed
-

-
 Performance counter stats for './new' (10 runs):

  0.888215 task-clock#0.831 CPUs utilized
 0 context-switches  #0.000 K/sec
 0 cpu-migrations#0.000 K/sec
   273 page-faults   #0.307 M/sec
 2,561,042 cycles#2.883 GHz
 2,804,468 instructions  #1.10  insns per cycle
   480,394 branches  #  540.853 M/sec
11,575 branch-misses #2.41% of all branches

   0.001068744 seconds time elapsed
-

Change-Id: I03726343d222bcd241c2c2a5a1670a672f8e5cb6
---
M src/kudu/rpc/connection.h
1 file changed, 7 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I03726343d222bcd241c2c2a5a1670a672f8e5cb6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 

[kudu-CR] [iwyu] another catch-up update

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [iwyu] another catch-up update
..


Patch Set 3:

(1 comment)

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

Line 24: #include 
> Makes sense, thanks.
Thank you for prompt and thoughtful review!


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR](branch-1.4.x) [java] KUDU-2103 Canonicalize hostnames in client

[Attila Bukor (Code Review)]

Attila Bukor has uploaded a new change for review.

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

Change subject: [java] KUDU-2103 Canonicalize hostnames in client
..

[java] KUDU-2103 Canonicalize hostnames in client

When Kerberos is used the service principal contains the
hostname of the service. This means that if a user wants to use
an alias for the master_addresses in a Kerberized environment
it won't be able to connect as the service principal name is not
found in the Kerberos database.

This patch adds canonicalization for the hostnames to make sure
the principal name the service ticket is requested for matches
the one used by the master servers.

Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/FakeDNS.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
4 files changed, 89 insertions(+), 6 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: branch-1.4.x
Gerrit-Owner: Attila Bukor 

[kudu-CR] Fix flakiness of kudu-admin-test TestMoveTablet

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Fix flakiness of kudu-admin-test TestMoveTablet
..


Patch Set 1:

(1 comment)

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

PS1, Line 36: Apparently the new election behavior made it more likely
: for the elections to have conflicts before getting a steady leader
> That seems like a problem too. From your debugging were you able to pinpoin
no, wasn't entirely clear. I figured it was the timing changes you made in your 
patch (bisect clearly pointed to the above-referenced commit as where it got 
worse).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] another catch-up update

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [iwyu] another catch-up update
..


Patch Set 3:

(1 comment)

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

Line 24: #include 
> As I understand, this file uses Messenger::ScheduleOnReactor() method, whic
Makes sense, thanks.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] another catch-up update

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged.

Change subject: [iwyu] another catch-up update
..


[iwyu] another catch-up update

This is just another catch-up patch after recent updates:
since the IWYU gerrit was not enabled for a while, some files
became not compliant with IWYU.  This patch fixes that,
keeping the code in IWYU-compliant state.

I also sneaked in a few updates on other files to remove them
from the 'muted' list in build-support/iwyu/iwyu-filter.awk

Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1
Reviewed-on: http://gerrit.cloudera.org:8080/7802
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M build-support/iwyu/iwyu-filter.awk
M src/kudu/cfile/index_btree.cc
M src/kudu/cfile/mt-bloomfile-test.cc
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/clock/logical_clock-test.cc
M src/kudu/codegen/codegen-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition_pruner-test.cc
M src/kudu/consensus/consensus_meta_manager-stress-test.cc
M src/kudu/consensus/consensus_peers.cc
M src/kudu/consensus/consensus_peers.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/block_manager_util.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/periodic-test.cc
M src/kudu/rpc/periodic.cc
M src/kudu/rpc/periodic.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/cert-test.cc
M src/kudu/security/crypto-test.cc
M src/kudu/security/tls_handshake-test.cc
M src/kudu/security/token-test.cc
M src/kudu/server/rpc_server-test.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mt-rowset_delta_compaction-test.cc
M src/kudu/tablet/mt-tablet-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tablet/transactions/transaction_tracker-test.cc
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tablet.cc
M src/kudu/tools/tool_action_tserver.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/util/compression/compression-test.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/subprocess.cc
68 files changed, 222 insertions(+), 143 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [iwyu] another catch-up update

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [iwyu] another catch-up update
..


Patch Set 3:

(1 comment)

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

Line 24: #include 
> I wrote this code and I'm curious why this is needed. There are no boost::f
As I understand, this file uses Messenger::ScheduleOnReactor() method, which 
takes boost::function as its first parameter.  If I understand correctly, to 
construct an object of that type, the information on the boost::function is 
needed for the compiler.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [security] avoid kerberos ticket renewal and only reacquire

[Sailesh Mukil (Code Review)]

Sailesh Mukil has uploaded a new change for review.

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

Change subject: [security] avoid kerberos ticket renewal and only reacquire
..

[security] avoid kerberos ticket renewal and only reacquire

It was found that if we use a file based credential cache that is
shared between the C++ side and the java side of a process, and we
encounter the specific edge case where we renew a ticket that has
less than 'ticket_lifetime' left before its 'renew_lifetime' expires,
the ticket is set to have a NULL 'renew_till' timestamp.

Eg:
ticket_lifetime = 10m
renew_lifetime = 100m

[current ticket being renewed] at '15:30:00'
endtime = '15:30:30'
renew_till = '15:31:00'

This ticket will be renewed and the renewed ticket will have the
following values:
endtime = '15:31:00'
renew_till = null

The Java krb5 library refuses to read these kinds of tickets which
have the RENEWABLE flag set but no 'renew_till' set, causing
unexpected failures.

Currently, the only way to work around this is to not renew tickets
at all and only always reacquire them. The reason for this is that
the Java side of a process or even another process may be running
its own renewal thread on the same credential cache for the same
principal(s). So even if we were to avoid renewing in this window,
the Java side could renew in this window, causing the above problem.
If we always reacquire the tickets, we're forcefully reseting this
window for that principal, thereby not allowing the Java side to hit
this bug.
The scenario where this bug played out is when using the kudu renewal
code in tandem with a hadoop process that use the same principals.

Also, currently there is no advantage we gain from just renewing the
tickets vs. reacquiring them, either in terms of security or
performance, since we login from a keytab.

Tracked on the Java side by:
http://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8186576

Change-Id: I8e5225de332ba785e3a73014b8418cfd4059fe07
---
M src/kudu/integration-tests/external_mini_cluster-test.cc
M src/kudu/security/init.cc
2 files changed, 19 insertions(+), 60 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I8e5225de332ba785e3a73014b8418cfd4059fe07
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 

[kudu-CR] Fix flakiness of kudu-admin-test TestMoveTablet

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: Fix flakiness of kudu-admin-test TestMoveTablet
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] Fix flakiness of kudu-admin-test TestMoveTablet

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Fix flakiness of kudu-admin-test TestMoveTablet
..


Patch Set 1: Code-Review+1

(1 comment)

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

PS1, Line 36: Apparently the new election behavior made it more likely
: for the elections to have conflicts before getting a steady leader
That seems like a problem too. From your debugging were you able to pinpoint 
why?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2098. Drop Spark 1 Support

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-2098. Drop Spark 1 Support
..


Patch Set 1:

So is the plan that this can merge after 1.5 branches?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5481cc0477f4d23d89b68ef510a6c9a2aa187537
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..


Patch Set 32:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

Line 132
> Adar earlier made the point "it shouldn't be part of the WritableBlock inte
There's no avoiding a downcast in LogBlockManager::CloseBlocks; we need it in 
order to call other non-interface methods on LogWritableBlock.

But, if you prefer to reduce the number of downcasts from two to one, some 
light refactoring could do that.


http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

PS32, Line 1217:   if (container_->read_only()) {
   : state_ = CLOSED;
   : if (container_->metrics()) {
   :   
container_->metrics()->generic_metrics.blocks_open_writing->Decrement();
   :   
container_->metrics()->generic_metrics.total_bytes_written->IncrementBy(
   :   block_length_);
   : }
   : return Status::Aborted("container $0 is read-only", 
container_->ToString());
   :   }
> Makes sense. I don't see a case we would want to Abort() the block if it is
I would prefer we didn't change API semantics at this point. Multiple Abort() 
calls in succession are legal; let's keep them that way (and ensure they no-op).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 32
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] gradle: target JRE 7 compatibility

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: gradle: target JRE 7 compatibility
..


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [security] avoid tickets with NULL 'renew till'

[Sailesh Mukil (Code Review)]

Sailesh Mukil has abandoned this change.

Change subject: [security] avoid tickets with NULL 'renew_till'
..


Abandoned

Turns out that this patch makes the issue mentioned in the commit message less 
severe, but still hits it occasionally.

The reason is that if the Kudu side renew tickets, the Java side (from 
hadoop-common in my testing) could also have its own renewal thread working on 
the same credential cache. And even if we're careful with our renewals, the 
java side could renew outside the ideal window that we're trying to maintain.

The only other way to work around this is to always reacquire instead of renew, 
thereby not giving a chance for the Java side renewal thread to renew outside 
this ideal window, as our reacquisition of the ticket would always reset the 
window for the new ticket in the credential cache.

Also, we don't have anything to gain in terms of performance or security from 
renewing vs. reacquiring when we login from a keytab.

I will put out another patch which removes the renewal code and only always 
reacquires.

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I59194af94838f680df4ce121a8dee526a876e369
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [security] avoid tickets with NULL 'renew till'

[Sailesh Mukil (Code Review)]

Sailesh Mukil has posted comments on this change.

Change subject: [security] avoid tickets with NULL 'renew_till'
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7770/2/src/kudu/security/init.cc
File src/kudu/security/init.cc:

PS2, Line 295: 
> I might be missing some crucial point here, but if renew_deadline is negati
You're right. That was my bad. In any case, I have to abandon this patch. After 
quite a lot of testing, it shows that this still doesn't work around the 
problem as I previously thought.

I've added a top level review comment as to why it doesn't work.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I59194af94838f680df4ce121a8dee526a876e369
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Fix flakiness of kudu-admin-test TestMoveTablet

[Todd Lipcon (Code Review)]

Hello Adar Dembo, Will Berkeley,

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

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

to review the following change.

Change subject: Fix flakiness of kudu-admin-test TestMoveTablet
..

Fix flakiness of kudu-admin-test TestMoveTablet

This fixes an issue in the admin tool for moving a tablet that would occur if
the tablet changed terms multiple times before settling during an election.

Previously, we would wait for a term change, and as soon as we saw any term
change, we'd wait for an operation in exactly that term to be committed. There
were two issues with the code:

1) It used cstate.has_leader_uuid() to check if there was a leader in the new
term. However, the server side actually sets this field to an empty string
rather than leaving it unset if there is no leader. So this check always
evaluated to true, meaning that we would proceed to looking for a committed op
in that term as soon as the term advanced, rather than waiting for an actual
leader to be elected.

2) In the case that somehow the leader changed a second time, the term could
be increased again while we are waiting for a committed operation. In that case
we'd be waiting for a committed op in exactly the term of the first leader
change we saw, rather than potentially refreshing our notion of the "current
term".

The patch fixes both issues by restructuring the loop a bit.

I additionally improved some logging in the consensus service and
implementation which I found helpful while debugging the issue.

This test became significantly more flaky (~20% in ASAN) after the commit
of 21b0f3d5e255760535e281efe5879fe657df1f1c which adjusted the election
timeout behavior. Apparently the new election behavior made it more likely
for the elections to have conflicts before getting a steady leader, which
exposed the above bug in the tool.

With the patch, I was able to run 500/500 successful in an ASAN build (vs
20% fail rate before).

Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177
---
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/tools/tool_action_tablet.cc
M src/kudu/tserver/tablet_service.cc
4 files changed, 54 insertions(+), 48 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-1894 fixed deadlock in client.Connection

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1894 fixed deadlock in client.Connection
..


Patch Set 4:

This looks good to me, but obviously this code is extremely fragile.  We should 
figure out a way to make this more robust down the line.  I took a quick look 
at the server implementation and it seems like it would be OK with out of order 
call IDs.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a0edc3e3accbcff60f2cde641ee470312bbd27a
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] gradle: target JRE 7 compatibility

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: gradle: target JRE 7 compatibility
..

gradle: target JRE 7 compatibility

Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29
---
M java/gradle.properties
M java/gradle/compile.gradle
2 files changed, 7 insertions(+), 5 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1894 fixed deadlock in client.Connection

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: KUDU-1894 fixed deadlock in client.Connection
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7765/4/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java:

Line 524: sendCallToWire(msg);
> yea but call_id is an int64 so it would require a single TCP connection to 
As far as I could see, call_id is int32_t, so I would expect it would rotate 
much faster :)

Yes, sure -- I agree it's better to be careful here.  I took a look at the code 
and I didn't find any place where non-ascending call_id (or sparse sequence of 
those) could break the code.  However, I might be missing something -- it's 
better to be safe than sorry.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a0edc3e3accbcff60f2cde641ee470312bbd27a
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..


Patch Set 32:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

Line 132
Looks like this is being retained as a method on both implementations, but to 
call it requires a downcast.  Should we keep it in the interface so that the 
downcast isn't necessary?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 32
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1894 fixed deadlock in client.Connection

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1894 fixed deadlock in client.Connection
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7765/4/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java:

Line 524: sendCallToWire(msg);
> Another thought is that the sequence anyway could go non-ascending order wh
yea but call_id is an int64 so it would require a single TCP connection to last 
for a few millennia for that to happen :) Let me take some time to ponder this 
patch a bit more.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1a0edc3e3accbcff60f2cde641ee470312bbd27a
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..


Patch Set 32:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

PS32, Line 1217:   if (container_->read_only()) {
   : state_ = CLOSED;
   : if (container_->metrics()) {
   :   
container_->metrics()->generic_metrics.blocks_open_writing->Decrement();
   :   
container_->metrics()->generic_metrics.total_bytes_written->IncrementBy(
   :   block_length_);
   : }
   : return Status::Aborted("container $0 is read-only", 
container_->ToString());
   :   }
> It's hard to tell whether it's possible to wind up calling Abort() twice on
Yeah, it does not hurt to add the checking. But looking at the flow, Abort() is 
only called when the state_ != CLOSED now. So instead maybe I can add a DCHECK 
of the state_ at the beginning of Abort()?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 32
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] [java] KUDU-2103 Canonicalize hostnames in client

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [java] KUDU-2103 Canonicalize hostnames in client
..


Patch Set 6:

Thanks for the fix, Attila. I think we should cherry pick this back to 
branch-1.4.x and branch-1.3.x, but it looks like it's not a clean cherry pick. 
Would you mind doing the backports? You can post to gerrit using 'git push 
gerrit HEAD:refs/for/branch-1.4.x' for example

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Attila Bukor 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [java] KUDU-2103 Canonicalize hostnames in client

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: [java] KUDU-2103 Canonicalize hostnames in client
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Attila Bukor 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [java] KUDU-2103 Canonicalize hostnames in client

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: [java] KUDU-2103 Canonicalize hostnames in client
..


[java] KUDU-2103 Canonicalize hostnames in client

When Kerberos is used the service principal contains the
hostname of the service. This means that if a user wants to use
an alias for the master_addresses in a Kerberized environment
it won't be able to connect as the service principal name is not
found in the Kerberos database.

This patch adds canonicalization for the hostnames to make sure
the principal name the service ticket is requested for matches
the one used by the master servers.

Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f
Reviewed-on: http://gerrit.cloudera.org:8080/7757
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/FakeDNS.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
4 files changed, 91 insertions(+), 6 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Attila Bukor 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR](branch-1.2.x) log block manager: use unsigned int for next block id

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: log block manager: use unsigned int for next_block_id_
..


log block manager: use unsigned int for next_block_id_

KUDU-1538 introduced 'next_block_id_' to keep track of unique block
ID that should be used for block creation. Currently, it is defined
as int64_t. However, it could be updated based on the value of
'max_block_id' which is uint64_t. Since block IDs are defined as
uint64_t both on disk (fs.proto) and in memory (block_id.h), it
makes more sense to treat 'next_block_id_' as uint64_t rather than
to convert it correctly to int64_t everywhere.

This patch changes the type of 'next_block_id_' to uint64_t to
avoid overflow due to conversion of unsigned int to int, which
can result in the reuse of an existing block ID. It does not add
a standalone test case because the failure is most likely to occur
in specific test-only scenarios.

Change-Id: Ib315b20719ef529331304df5c56c4242902524d4
Reviewed-on: http://gerrit.cloudera.org:8080/7796
Reviewed-by: Adar Dembo 
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
(cherry picked from commit b37bde72c47370293827a306532d9be15b1c5f40)
Reviewed-on: http://gerrit.cloudera.org:8080/7799
---
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/log_block_manager.h
2 files changed, 2 insertions(+), 2 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib315b20719ef529331304df5c56c4242902524d4
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: branch-1.2.x
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR](branch-1.3.x) log block manager: use unsigned int for next block id

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: log block manager: use unsigned int for next_block_id_
..


log block manager: use unsigned int for next_block_id_

KUDU-1538 introduced 'next_block_id_' to keep track of unique block
ID that should be used for block creation. Currently, it is defined
as int64_t. However, it could be updated based on the value of
'max_block_id' which is uint64_t. Since block IDs are defined as
uint64_t both on disk (fs.proto) and in memory (block_id.h), it
makes more sense to treat 'next_block_id_' as uint64_t rather than
to convert it correctly to int64_t everywhere.

This patch changes the type of 'next_block_id_' to uint64_t to
avoid overflow due to conversion of unsigned int to int, which
can result in the reuse of an existing block ID. It does not add
a standalone test case because the failure is most likely to occur
in specific test-only scenarios.

Change-Id: Ib315b20719ef529331304df5c56c4242902524d4
Reviewed-on: http://gerrit.cloudera.org:8080/7796
Reviewed-by: Adar Dembo 
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
(cherry picked from commit b37bde72c47370293827a306532d9be15b1c5f40)
Reviewed-on: http://gerrit.cloudera.org:8080/7798
---
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/log_block_manager.h
2 files changed, 2 insertions(+), 2 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib315b20719ef529331304df5c56c4242902524d4
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: branch-1.3.x
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR](branch-1.4.x) log block manager: use unsigned int for next block id

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: log block manager: use unsigned int for next_block_id_
..


log block manager: use unsigned int for next_block_id_

KUDU-1538 introduced 'next_block_id_' to keep track of unique block
ID that should be used for block creation. Currently, it is defined
as int64_t. However, it could be updated based on the value of
'max_block_id' which is uint64_t. Since block IDs are defined as
uint64_t both on disk (fs.proto) and in memory (block_id.h), it
makes more sense to treat 'next_block_id_' as uint64_t rather than
to convert it correctly to int64_t everywhere.

This patch changes the type of 'next_block_id_' to uint64_t to
avoid overflow due to conversion of unsigned int to int, which
can result in the reuse of an existing block ID. It does not add
a standalone test case because the failure is most likely to occur
in specific test-only scenarios.

Change-Id: Ib315b20719ef529331304df5c56c4242902524d4
Reviewed-on: http://gerrit.cloudera.org:8080/7796
Reviewed-by: Adar Dembo 
Reviewed-by: Todd Lipcon 
Tested-by: Kudu Jenkins
(cherry picked from commit b37bde72c47370293827a306532d9be15b1c5f40)
Reviewed-on: http://gerrit.cloudera.org:8080/7797
---
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/log_block_manager.h
2 files changed, 2 insertions(+), 2 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib315b20719ef529331304df5c56c4242902524d4
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: branch-1.4.x
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [iwyu] another catch-up update

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [iwyu] another catch-up update
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7802/2/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:

Line 45: #include "kudu/gutil/casts.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] another catch-up update

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [iwyu] another catch-up update
..

[iwyu] another catch-up update

This is just another catch-up patch after recent updates:
since the IWYU gerrit was not enabled for a while, some files
became not compliant with IWYU.  This patch fixes that,
keeping the code in IWYU-compliant state.

I also sneaked in a few updates on other files to remove them
from the 'muted' list in build-support/iwyu/iwyu-filter.awk

Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1
---
M build-support/iwyu/iwyu-filter.awk
M src/kudu/cfile/index_btree.cc
M src/kudu/cfile/mt-bloomfile-test.cc
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/clock/logical_clock-test.cc
M src/kudu/codegen/codegen-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition_pruner-test.cc
M src/kudu/consensus/consensus_meta_manager-stress-test.cc
M src/kudu/consensus/consensus_peers.cc
M src/kudu/consensus/consensus_peers.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/block_manager_util.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/periodic-test.cc
M src/kudu/rpc/periodic.cc
M src/kudu/rpc/periodic.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/cert-test.cc
M src/kudu/security/crypto-test.cc
M src/kudu/security/tls_handshake-test.cc
M src/kudu/security/token-test.cc
M src/kudu/server/rpc_server-test.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mt-rowset_delta_compaction-test.cc
M src/kudu/tablet/mt-tablet-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tablet/transactions/transaction_tracker-test.cc
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tablet.cc
M src/kudu/tools/tool_action_tserver.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/util/compression/compression-test.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/subprocess.cc
68 files changed, 222 insertions(+), 143 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [iwyu] another catch-up update

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [iwyu] another catch-up update
..


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7802/1/src/kudu/cfile/index_btree.cc
File src/kudu/cfile/index_btree.cc:

Line 26: #include "kudu/cfile/cfile_writer.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


http://gerrit.cloudera.org:8080/#/c/7802/1/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:

Line 39: #include "kudu/gutil/port.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


http://gerrit.cloudera.org:8080/#/c/7802/1/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:

Line 45: #include "kudu/gutil/casts.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


http://gerrit.cloudera.org:8080/#/c/7802/1/src/kudu/server/rpc_server-test.cc
File src/kudu/server/rpc_server-test.cc:

Line 25: #include "kudu/rpc/messenger.h"
> warning: #includes are not sorted properly [llvm-include-order]
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [iwyu] another catch-up update

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [iwyu] another catch-up update
..

[iwyu] another catch-up update

This is just another catch-up patch after recent updates:
since the IWYU gerrit was not enabled for a while, some files
became not compliant with IWYU.  This patch fixes that,
keeping the code in IWYU-compliant state.

I also sneaked in a few updates on other files to remove them
from the 'muted' list in build-support/iwyu/iwyu-filter.awk

Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1
---
M build-support/iwyu/iwyu-filter.awk
M src/kudu/cfile/index_btree.cc
M src/kudu/cfile/mt-bloomfile-test.cc
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/clock/logical_clock-test.cc
M src/kudu/codegen/codegen-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition_pruner-test.cc
M src/kudu/consensus/consensus_meta_manager-stress-test.cc
M src/kudu/consensus/consensus_peers.cc
M src/kudu/consensus/consensus_peers.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/block_manager_util.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/periodic-test.cc
M src/kudu/rpc/periodic.cc
M src/kudu/rpc/periodic.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/cert-test.cc
M src/kudu/security/crypto-test.cc
M src/kudu/security/tls_handshake-test.cc
M src/kudu/security/token-test.cc
M src/kudu/server/rpc_server-test.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mt-rowset_delta_compaction-test.cc
M src/kudu/tablet/mt-tablet-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tablet/transactions/transaction_tracker-test.cc
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tablet.cc
M src/kudu/tools/tool_action_tserver.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/util/compression/compression-test.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/subprocess.cc
68 files changed, 223 insertions(+), 144 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [iwyu] another catch-up update

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: [iwyu] another catch-up update
..

[iwyu] another catch-up update

This is just another catch-up patch after recent updates:
since the IWYU gerrit was not enabled for a while, some files
became not compliant with IWYU.  This patch fixes that,
keeping the code in IWYU-compliant state.

I also sneaked in a few updates on other files to remove them
from the 'muted' list in build-support/iwyu/iwyu-filter.awk

Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1
---
M build-support/iwyu/iwyu-filter.awk
M src/kudu/cfile/index_btree.cc
M src/kudu/cfile/mt-bloomfile-test.cc
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/clock/logical_clock-test.cc
M src/kudu/codegen/codegen-test.cc
M src/kudu/common/partition-test.cc
M src/kudu/common/partition_pruner-test.cc
M src/kudu/consensus/consensus_meta_manager-stress-test.cc
M src/kudu/consensus/consensus_peers.cc
M src/kudu/consensus/consensus_peers.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/block_manager_util.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/integration-tests/disk_failure-itest.cc
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/negotiation-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/periodic-test.cc
M src/kudu/rpc/periodic.cc
M src/kudu/rpc/periodic.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
M src/kudu/security/ca/cert_management-test.cc
M src/kudu/security/cert-test.cc
M src/kudu/security/crypto-test.cc
M src/kudu/security/tls_handshake-test.cc
M src/kudu/security/token-test.cc
M src/kudu/server/rpc_server-test.cc
M src/kudu/tablet/all_types-scan-correctness-test.cc
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mt-rowset_delta_compaction-test.cc
M src/kudu/tablet/mt-tablet-test.cc
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/tablet/transactions/transaction_tracker-test.cc
M src/kudu/tools/tool_action.cc
M src/kudu/tools/tool_action_master.cc
M src/kudu/tools/tool_action_tablet.cc
M src/kudu/tools/tool_action_tserver.cc
M src/kudu/tserver/tablet_copy_source_session-test.cc
M src/kudu/util/compression/compression-test.cc
M src/kudu/util/file_cache-stress-test.cc
M src/kudu/util/subprocess.cc
68 files changed, 220 insertions(+), 141 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 

[kudu-CR] [java] KUDU-2103 Canonicalize hostnames in client

[Attila Bukor (Code Review)]

Attila Bukor has posted comments on this change.

Change subject: [java] KUDU-2103 Canonicalize hostnames in client
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7757/3/java/kudu-client/src/test/java/org/apache/kudu/client/FakeDNS.java
File java/kudu-client/src/test/java/org/apache/kudu/client/FakeDNS.java:

PS3, Line 93:   if (forwardResolutions.containsKey(host)) {
: return new InetAddress[]{forwardResolutions.get(host)};
:   }
> nit: to avoid double lookup, restructure as:
shouldn't run an actual lookup, but changed it


http://gerrit.cloudera.org:8080/#/c/7757/4/java/kudu-client/src/test/java/org/apache/kudu/client/FakeDNS.java
File java/kudu-client/src/test/java/org/apache/kudu/client/FakeDNS.java:

PS4, Line 56:   }
: 
:   public Map getReverseResolutions() {
: return reverseResolutions;
:   }
: 
:   /
> sorry, one more nit I missed: we always define all members up front before 
I did but unfortunately it's not configured to run on the tests directory, only 
on main. (Tried to post it to your last review, but only saved it as a draft... 
need to get used to this UI...)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Attila Bukor 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [java] KUDU-2103 Canonicalize hostnames in client

[Attila Bukor (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [java] KUDU-2103 Canonicalize hostnames in client
..

[java] KUDU-2103 Canonicalize hostnames in client

When Kerberos is used the service principal contains the
hostname of the service. This means that if a user wants to use
an alias for the master_addresses in a Kerberized environment
it won't be able to connect as the service principal name is not
found in the Kerberos database.

This patch adds canonicalization for the hostnames to make sure
the principal name the service ticket is requested for matches
the one used by the master servers.

Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f
---
M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java
M java/kudu-client/src/test/java/org/apache/kudu/client/FakeDNS.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java
4 files changed, 91 insertions(+), 6 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Attila Bukor 
Gerrit-Reviewer: Attila Bukor 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..


Patch Set 32:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

PS32, Line 1217:   if (container_->read_only()) {
   : state_ = CLOSED;
   : if (container_->metrics()) {
   :   
container_->metrics()->generic_metrics.blocks_open_writing->Decrement();
   :   
container_->metrics()->generic_metrics.total_bytes_written->IncrementBy(
   :   block_length_);
   : }
   : return Status::Aborted("container $0 is read-only", 
container_->ToString());
   :   }
> Do we need to check if state_ is already CLOSED and if so make this a no-op
It's hard to tell whether it's possible to wind up calling Abort() twice on a 
read-only container given the constraints required to get a container to become 
read-only, but Mike's suggestion certainly won't hurt.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 32
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy
..


Patch Set 5:

(4 comments)

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

PS1, Line 13: to keep track of block data disk synchronization count.
: 
: I did a m
> Done
What about the disk setup? How many disks did each tserver have? Did they share 
disks?


http://gerrit.cloudera.org:8080/#/c/7701/5/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 1870: s = env_->SyncDir(container.data_dir()->dir());
Should also track this, no?


http://gerrit.cloudera.org:8080/#/c/7701/5/src/kudu/tserver/tablet_copy_client-test.cc
File src/kudu/tserver/tablet_copy_client-test.cc:

Line 91: MetricRegistry registry;
I'm surprised it's OK for the registry to go out of scope and be destroyed 
while the MetricEntity is still alive.


http://gerrit.cloudera.org:8080/#/c/7701/5/src/kudu/tserver/tablet_copy_client.h
File src/kudu/tserver/tablet_copy_client.h:

PS5, Line 178: Data block is opened with options.
I don't understand what this remnant is trying to convey.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes