[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

[Dan Burkert (Code Review)]

Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 
Todd Lipcon,

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

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

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

Change subject: KUDU-2191 (2/n): Hive Metastore client
..

KUDU-2191 (2/n): Hive Metastore client

This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.

The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.

- Thrift has been added as a dependency, and a mechanism for performing
  Thrift codegen at compile time has been added (see FindThrift.cmake,
  based on FindProtobuf.cmake)

- Bison has been added as a build-time dependency, because the system
  bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.

- Hive and Hadoop have been added to thirdparty as test-only dependencies.

- A Hive MetaStore external mini server is included for testing. See
  mini_hms.[h|cc].

- The Kudu Metastore plugin is compiled from CMake as a standalone jar
  for loading into the HMS mini server.

Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/iwyu/iwyu-filter.awk
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,920 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/31
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 31
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7053 )

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Patch Set 30:

(14 comments)

I've added TODOs to expand the error handling behavior and documentation in 
HmsClient.  I'd like for these things to be tested thoroughly, but this commit 
is already quite large when considering the build changes, so I'd like to leave 
it to a follow-up commit.

http://gerrit.cloudera.org:8080/#/c/7053/30/build-support/run_dist_test.py
File build-support/run_dist_test.py:

http://gerrit.cloudera.org:8080/#/c/7053/30/build-support/run_dist_test.py@128
PS30, Line 128:   env['HIVE_HOME'] = glob.glob(os.path.join(ROOT, 
"thirdparty/src/apache-hive-*-bin"))[0]
  :   env['HADOOP_HOME'] = glob.glob(os.path.join(ROOT, 
"thirdparty/src/hadoop-*"))[0]
> when we end up revving these dependencies it seems likely we'll end up with
Done


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/CMakeLists.txt
File src/kudu/hms/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/CMakeLists.txt@40
PS30, Line 40: execute_process(COMMAND ln -nsf
 : 
"${CMAKE_SOURCE_DIR}/thirdparty/installed/common/opt/hive"
 : "${EXECUTABLE_OUTPUT_PATH}/hive-home")
 : execute_process(COMMAND ln -nsf
 : 
"${CMAKE_SOURCE_DIR}/thirdparty/installed/common/opt/hadoop"
 : "${EXECUTABLE_OUTPUT_PATH}/hadoop-home")
 : execute_process(COMMAND ln -nsf
 : "${JAVA_HOME}"
 : "${EXECUTABLE_OUTPUT_PATH}/java-home")
> does it make more sense to do these in build-thirdparty? even though it's n
I don't think it's possible, since we can't know what directory the user will 
build Kudu in during the thirdparty build.


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hive_metastore.thrift
File src/kudu/hms/hive_metastore.thrift:

http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hive_metastore.thrift@21
PS30, Line 21: # DO NOT MODIFY! Copied from
> should we be setting any C++ specific options like 'moveable types'?
I added moveable_types to FindThrift generation.  I don't think it really buys 
us much since all the service interfaces still takes args by const ref, but I 
guess it helps for building the arguments to begin with.


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:

http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client-test.cc@103
PS30, Line 103:   ASSERT_TRUE(CreateTable(, database_name, table_name, 
"").IsRuntimeError());
> i think it's worth ASSERT_STR_MATCHES the error string here since RuntimeEr
Done


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:

http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@60
PS30, Line 60:   // can not be reached, an error is returned.
> is there some default timeout, etc?
Not currently, I've added a TODO.


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@73
PS30, Line 73:   // returned.
> it's probably worth a class-wide comment on what the errors returned will b
I've added a TODO.  I think we should have test coverage if we're going to 
document it, and I'd like to leave that to a follow-up commit.


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@85
PS30, Line 85: bool cascade = false,
 :   bool delete_data = true)
> would this be better off as a bitset of flags?
Not in my opinion.  What advantage would that have?  This method will only be 
used in tests, per the current design.


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@121
PS30, Line 121:int32_t max_events,
> do we have any idea of the expected size of these events? wondering if we n
They can be very large for HDFS tables with a lot of partitions.  There's 
currently no way to filter the events to just include Kudu tables, 
unfortunately.


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@124
PS30, Line 124:   // Deserializes a JSON encoded table.
> can you add a little more context as to where one would see the JSON-encode
Done


http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.h@22
PS29, Line 22: #include 
> Could you at least add the IWYU pragma, please?  It should be something lik
sure, I completely forgot pragmas were an option.


http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:

http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc@78
PS30, Line 78: IOError
> would RemoteError 

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

[Dan Burkert (Code Review)]

Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 
Todd Lipcon,

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

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

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

Change subject: KUDU-2191 (2/n): Hive Metastore client
..

KUDU-2191 (2/n): Hive Metastore client

This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.

The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.

- Thrift has been added as a dependency, and a mechanism for performing
  Thrift codegen at compile time has been added (see FindThrift.cmake,
  based on FindProtobuf.cmake)

- Bison has been added as a build-time dependency, because the system
  bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.

- Hive and Hadoop have been added to thirdparty as test-only dependencies.

- A Hive MetaStore external mini server is included for testing. See
  mini_hms.[h|cc].

- The Kudu Metastore plugin is compiled from CMake as a standalone jar
  for loading into the HMS mini server.

Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/iwyu/iwyu-filter.awk
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,924 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/32
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 32
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

[Dan Burkert (Code Review)]

Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 
Todd Lipcon,

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

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

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

Change subject: KUDU-2191 (2/n): Hive Metastore client
..

KUDU-2191 (2/n): Hive Metastore client

This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.

The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.

- Thrift has been added as a dependency, and a mechanism for performing
  Thrift codegen at compile time has been added (see FindThrift.cmake,
  based on FindProtobuf.cmake)

- Bison has been added as a build-time dependency, because the system
  bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.

- Hive and Hadoop have been added to thirdparty as test-only dependencies.

- A Hive MetaStore external mini server is included for testing. See
  mini_hms.[h|cc].

- The Kudu Metastore plugin is compiled from CMake as a standalone jar
  for loading into the HMS mini server.

Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/iwyu/iwyu-filter.awk
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,928 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/35
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 35
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] python: upgrade pip when building client and restore old workarounds

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8406 )

Change subject: python: upgrade pip when building client and restore old 
workarounds
..

python: upgrade pip when building client and restore old workarounds

It appears that pypi recently deactivated their http:// endpoint, which is
the default in the older versions of pip found on el6. As a result, pip
installation requests fail:

  Downloading/unpacking pytest (from -r requirements.txt (line 1))
Cannot fetch index base URL http://pypi.python.org/simple/
Could not find any downloads that satisfy the requirement pytest (from -r 
requirements.txt (line 1))
  No distributions at all found for pytest (from -r requirements.txt (line 1))

Passing -i with the https:// endpoint works for the dependencies being
installed, but it doesn't appear to be passed down correctly to transitive
dependencies, and they fail to install with an inscrutable error [1].

So instead, let's go back to the old approach where we first upgrade pip.
This means effectively reverting [2] and restoring [3], but such is life.

I tested this by running build-and-test.sh on an el6 machine that created
virtualenvs containing pip 1.1 and setuptools 0.6c11.

1. 
https://stackoverflow.com/questions/5178535/easy-install-libmysqld-dev-error-nonetype-object-has-no-attribute-clone
2. 
https://github.com/apache/kudu/commit/b198ed8ff6a603816892c8bc7fd80679a014aaf1
3. 
https://github.com/apache/kudu/commit/d0acb55510c0552605953e07740f46d6be66c9c1

Change-Id: Ia75b65d66a1559ddaefb53dc1e67837faf9e7e6a
Reviewed-on: http://gerrit.cloudera.org:8080/8406
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans 
---
M build-support/jenkins/build-and-test.sh
M python/requirements.txt
2 files changed, 42 insertions(+), 3 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Jean-Daniel Cryans: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia75b65d66a1559ddaefb53dc1e67837faf9e7e6a
Gerrit-Change-Number: 8406
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] python: upgrade pip when building client and restore old workarounds

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8406 )

Change subject: python: upgrade pip when building client and restore old 
workarounds
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia75b65d66a1559ddaefb53dc1e67837faf9e7e6a
Gerrit-Change-Number: 8406
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 30 Oct 2017 16:02:18 +
Gerrit-HasComments: No

[kudu-CR] [java client] improve AsyncKuduScanner logging

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8382 )

Change subject: [java client] improve AsyncKuduScanner logging
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8382/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
File 
java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java:

http://gerrit.cloudera.org:8080/#/c/8382/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@727
PS3, Line 727: ret += ", lastPrimaryKey = " + 
Bytes.pretty(lastPrimaryKey);
> Hmm, you are right. Do you think it makes sense to log a hashed value for d
I'd lean on the side of not exposing it, since it's somewhat complicated to use 
a crypto-secure hash (calling hashCode() isn't sufficient).  But if you think 
it's worth the effort in terms of increased debug-ability, then go for it.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I90ffcd01e7f99f3090fa118092fc303e06fb92dc
Gerrit-Change-Number: 8382
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Oct 2017 16:07:16 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7053 )

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Patch Set 31:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7053/31//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/7053/31//COMMIT_MSG@4
PS31, Line 4: d...@danburkert.com
revert



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 31
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Oct 2017 14:56:46 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

[Dan Burkert (Code Review)]

Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 
Todd Lipcon,

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

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

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

Change subject: KUDU-2191 (2/n): Hive Metastore client
..

KUDU-2191 (2/n): Hive Metastore client

This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.

The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.

- Thrift has been added as a dependency, and a mechanism for performing
  Thrift codegen at compile time has been added (see FindThrift.cmake,
  based on FindProtobuf.cmake)

- Bison has been added as a build-time dependency, because the system
  bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.

- Hive and Hadoop have been added to thirdparty as test-only dependencies.

- A Hive MetaStore external mini server is included for testing. See
  mini_hms.[h|cc].

- The Kudu Metastore plugin is compiled from CMake as a standalone jar
  for loading into the HMS mini server.

Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/iwyu/iwyu-filter.awk
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,926 insertions(+), 6 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 33
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

[Dan Burkert (Code Review)]

Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 
Todd Lipcon,

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

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

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

Change subject: KUDU-2191 (2/n): Hive Metastore client
..

KUDU-2191 (2/n): Hive Metastore client

This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.

The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.

- Thrift has been added as a dependency, and a mechanism for performing
  Thrift codegen at compile time has been added (see FindThrift.cmake,
  based on FindProtobuf.cmake)

- Bison has been added as a build-time dependency, because the system
  bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.

- Hive and Hadoop have been added to thirdparty as test-only dependencies.

- A Hive MetaStore external mini server is included for testing. See
  mini_hms.[h|cc].

- The Kudu Metastore plugin is compiled from CMake as a standalone jar
  for loading into the HMS mini server.

Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/iwyu/iwyu-filter.awk
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,928 insertions(+), 7 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 34
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

[Dan Burkert (Code Review)]

Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, 
Todd Lipcon,

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

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

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

Change subject: KUDU-2191 (2/n): Hive Metastore client
..

KUDU-2191 (2/n): Hive Metastore client

This patch lays the groundwork for integrating the Kudu catalog with the
Hive MetaStore.

The focus of this patch is a Kudu-specific C++ HMS client
(hms_client.[h|cc]) in a new hms module. This client provides bindings
for the Hive Metastore APIs that Kudu will use in follow-up commits.

- Thrift has been added as a dependency, and a mechanism for performing
  Thrift codegen at compile time has been added (see FindThrift.cmake,
  based on FindProtobuf.cmake)

- Bison has been added as a build-time dependency, because the system
  bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10.

- Hive and Hadoop have been added to thirdparty as test-only dependencies.

- A Hive MetaStore external mini server is included for testing. See
  mini_hms.[h|cc].

- The Kudu Metastore plugin is compiled from CMake as a standalone jar
  for loading into the HMS mini server.

Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/iwyu/iwyu-filter.awk
M build-support/run_dist_test.py
A cmake_modules/FindJavaHome.cmake
A cmake_modules/FindThrift.cmake
A src/kudu/hms/CMakeLists.txt
A src/kudu/hms/hive_metastore.thrift
A src/kudu/hms/hms_client-test.cc
A src/kudu/hms/hms_client.cc
A src/kudu/hms/hms_client.h
A src/kudu/hms/mini_hms.cc
A src/kudu/hms/mini_hms.h
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
18 files changed, 2,928 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/36
--
To view, visit http://gerrit.cloudera.org:8080/7053
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 36
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7053 )

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Patch Set 30:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7053/30/build-support/run_dist_test.py
File build-support/run_dist_test.py:

http://gerrit.cloudera.org:8080/#/c/7053/30/build-support/run_dist_test.py@128
PS30, Line 128:   env['HIVE_HOME'] = glob.glob(os.path.join(ROOT, 
"thirdparty/src/apache-hive-*-bin"))[0]
  :   env['HADOOP_HOME'] = glob.glob(os.path.join(ROOT, 
"thirdparty/src/hadoop-*"))[0]
> Done
Woops, turns out this was correct the first time.  dist_test.py ensures that 
only the correct versions of hive and hadoop are copied, because they use the 
build/latest/bin/[hadoop-home|hive-home] symlinks.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 30
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Oct 2017 16:15:36 +
Gerrit-HasComments: Yes

[kudu-CR] [webui] Add templates for tserver webui

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8307 )

Change subject: [webui] Add templates for tserver webui
..


Patch Set 7:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8307/7/src/kudu/tserver/tserver_path_handlers.h
File src/kudu/tserver/tserver_path_handlers.h:

http://gerrit.cloudera.org:8080/#/c/8307/7/src/kudu/tserver/tserver_path_handlers.h@27
PS7, Line 27: template  class scoped_refptr;
> warning: invalid case style for class 'scoped_refptr' [readability-identifi
:(


http://gerrit.cloudera.org:8080/#/c/8307/7/src/kudu/tserver/tserver_path_handlers.cc
File src/kudu/tserver/tserver_path_handlers.cc:

http://gerrit.cloudera.org:8080/#/c/8307/7/src/kudu/tserver/tserver_path_handlers.cc@170
PS7, Line 170:   transaction_json["desc"] = 
std::move(inflight_tx.description());
> warning: std::move of the const expression has no effect; remove std::move(
Done


http://gerrit.cloudera.org:8080/#/c/8307/7/src/kudu/tserver/tserver_path_handlers.cc@172
PS7, Line 172: transaction_json["trace"] = 
std::move(inflight_tx.trace_buffer());
> warning: std::move of the const expression has no effect; remove std::move(
Done


http://gerrit.cloudera.org:8080/#/c/8307/7/src/kudu/tserver/tserver_path_handlers.cc@214
PS7, Line 214:   map tablet_statuses;
> nit: maybe rename this to tablet_states?
Done


http://gerrit.cloudera.org:8080/#/c/8307/7/src/kudu/tserver/tserver_path_handlers.cc@215
PS7, Line 215:   for (const scoped_refptr& replica : replicas) {
> Can this loop be merged with the other loop over `replicas`? Seems like the
Done


http://gerrit.cloudera.org:8080/#/c/8307/7/src/kudu/tserver/tserver_path_handlers.cc@240
PS7, Line 240:   tablet_detail_json["target"] = Substitute("/tablet?id=$0", 
status.tablet_id());
> Does the /tablet page exist if replica exists but the tablet doesn't exist?
Technically the /tablet page does exist if the replica exists but the tablet 
doesn't, but there's not going to be anything useful about it anymore.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99e08be9aa8abddd51ada61683b6f75190a00b5c
Gerrit-Change-Number: 8307
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Mon, 30 Oct 2017 22:10:54 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8304 )

Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8304/9/src/kudu/mini-cluster/external_mini_cluster-test.cc
File src/kudu/mini-cluster/external_mini_cluster-test.cc:

http://gerrit.cloudera.org:8080/#/c/8304/9/src/kudu/mini-cluster/external_mini_cluster-test.cc@28
PS9, Line 28: #include "kudu/gutil/strings/util.h" // IWYU pragma: keep
> What's util.h used for?
HasPrefixString



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0
Gerrit-Change-Number: 8304
Gerrit-PatchSet: 9
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Oct 2017 22:41:57 +
Gerrit-HasComments: Yes

[kudu-CR] Add a macro form for ScopedCleanup

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8416 )

Change subject: Add a macro form for ScopedCleanup
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8416/1/src/kudu/util/scoped_cleanup.h
File src/kudu/util/scoped_cleanup.h:

http://gerrit.cloudera.org:8080/#/c/8416/1/src/kudu/util/scoped_cleanup.h@31
PS1, Line 31: // NOTE: in the case that you want to cancel the cleanup, use the 
more verbose
: // (non-macro) form below.
> Worth mentioning that this form automatically captures everything by ref?
eh, given that the definition is only a single line and right here next to the 
comment, I think it's a bit redundant.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb72c27824def3c6a6d98c56b46cdd620f651e0e
Gerrit-Change-Number: 8416
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Oct 2017 21:28:51 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2205. Improve error message for failed globs

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8415 )

Change subject: KUDU-2205. Improve error message for failed globs
..

KUDU-2205. Improve error message for failed globs

This fixes the Env::Glob function to give a more understandable error
message on typical issues such as permission denied. This can improve
the diagnosability in the case that a server is started with an
inaccessible log directory.

Change-Id: Ib7e487b5f6b24c2a2bd66e33f5b51a31e6585657
Reviewed-on: http://gerrit.cloudera.org:8080/8415
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
---
M src/kudu/util/env-test.cc
M src/kudu/util/env_posix.cc
2 files changed, 21 insertions(+), 1 deletion(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib7e487b5f6b24c2a2bd66e33f5b51a31e6585657
Gerrit-Change-Number: 8415
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] Add a macro form for ScopedCleanup

[Todd Lipcon (Code Review)]

Hello Adar Dembo,

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

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

to review the following change.


Change subject: Add a macro form for ScopedCleanup
..

Add a macro form for ScopedCleanup

This adds a new macro SCOPED_CLEANUP({ ... }) which is a shorter
form of 'auto cleanup = MakeScopedCleanup([&] { ... })

I also changed over a bunch of call sites to use it (those that were
easily discovered by a perl regex). We can switch over the remaining
ones as we run across them.

Change-Id: Idb72c27824def3c6a6d98c56b46cdd620f651e0e
---
M src/kudu/fs/log_block_manager.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/client-negotiation-failover-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/security-unknown-tsk-itest.cc
M src/kudu/integration-tests/tombstoned_voting-stress-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/init.cc
M src/kudu/security/openssl_util.cc
M src/kudu/util/env-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/scoped_cleanup-test.cc
M src/kudu/util/scoped_cleanup.h
M src/kudu/util/threadpool-test.cc
16 files changed, 53 insertions(+), 26 deletions(-)



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

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

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7053 )

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Patch Set 36:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7053/36/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:

http://gerrit.cloudera.org:8080/#/c/7053/36/src/kudu/hms/mini_hms.h@30
PS36, Line 30: #include "kudu/util/subprocess.h" // IWYU pragma: keep
Any particular reason you don't want to move MiniHms constructor into 
mini_hms.cc, thus allowing you to cleanly forward declare Subprocess and avoid 
this inclusion? Alexey and I both suggested that back in PS29/PS30.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 36
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Oct 2017 22:12:48 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7053 )

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Patch Set 36:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7053/36/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:

http://gerrit.cloudera.org:8080/#/c/7053/36/src/kudu/hms/mini_hms.h@30
PS36, Line 30: #include "kudu/util/subprocess.h" // IWYU pragma: keep
> Any particular reason you don't want to move MiniHms constructor into mini_
I don't like the idea of making the code more complex in order to satisfy IWYU.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 36
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Oct 2017 22:41:15 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8304 )

Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8304/9/src/kudu/mini-cluster/external_mini_cluster-test.cc
File src/kudu/mini-cluster/external_mini_cluster-test.cc:

http://gerrit.cloudera.org:8080/#/c/8304/9/src/kudu/mini-cluster/external_mini_cluster-test.cc@28
PS9, Line 28: #include "kudu/gutil/strings/util.h" // IWYU pragma: keep
What's util.h used for?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0
Gerrit-Change-Number: 8304
Gerrit-PatchSet: 9
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Oct 2017 22:15:10 +
Gerrit-HasComments: Yes

[kudu-CR] [mini-cluster] allow 18-bit PID in LOOPBACK bind mode

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: [mini-cluster] allow 18-bit PID in LOOPBACK bind mode
..

[mini-cluster] allow 18-bit PID in LOOPBACK bind mode

This changelist allows to use 18-bit PID for binding
in LOOPBACK mode.  Prior to this patch, the minicluster supported
PIDs to be up to 16 bits wide.

While expanding the range for PIDs, this also narrows down the limit
on maximum number of 'indexed servers' to 62 (it was 262 prior to this
change).  Hopefully, that's enough for our minicluster tests.

The incentive to put up this patch was change of the max PID up to
147456 in /proc/sys/kernel/pid_max after recent restart of one
of our build servers.

Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130
---
M src/kudu/mini-cluster/mini_cluster.cc
1 file changed, 20 insertions(+), 7 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130
Gerrit-Change-Number: 8422
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [mini-cluster] allow 18-bit PID in LOOPBACK bind mode

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: [mini-cluster] allow 18-bit PID in LOOPBACK bind mode
..

[mini-cluster] allow 18-bit PID in LOOPBACK bind mode

This changelist allows to use 18-bit PIDs for binding in LOOPBACK mode.
Prior to this patch, the minicluster could not use PIDs wider
than 16 bits.

While expanding the range of acceptable PIDs, this changelist also
decreases the maximum allowed number of 'indexed servers' from 254
to 62.  As of now, that's enough for our minicluster-based tests.

The incentive to put up this patch was change of the max PID
up to 147456 in /proc/sys/kernel/pid_max after recent restart of
one of our build servers.

Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130
---
M src/kudu/mini-cluster/mini_cluster.cc
1 file changed, 20 insertions(+), 7 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130
Gerrit-Change-Number: 8422
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] cfile set: make FindRow more robust to errors

[Andrew Wong (Code Review)]

Andrew Wong has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8423


Change subject: cfile_set: make FindRow more robust to errors
..

cfile_set: make FindRow more robust to errors

Previously, FindRow() would not handle certain error cases well. I.e.
when seeking on a key, it wouldn't consider all errors before returning
the seek results.

For example, currently, FindRow() scans over CFiles and if 1) it hits a
Status::NotFound() error, or 2) the key search returned without being
an exact match, it returns success, returning the row is not present.

However, 2) does not take into account the OK-ness of the returned
scans, and thus may return healthily saying it couldn't find the row,
even though this may not be the case. This allows the currently-running
operation to continue under the potentially incorrect assumption that
the key is not present, when it should actually return with the
appropriate error.

In the case of DuplicatingRowSet::MutateRow(), this is particularly
nasty: it expects operations to be mirrored to two separate rowsets, and
the above scenario may present itself as a fatal failure to do so.

In practice, this hasn't been an issue because we might only expect
various fs-level errors (e.g. corruption, disk errors) here, which we
already don't handle gracefully. However, this needs to change to be
robust to disk failures.

Similarly, FindRow() will now return early if it hits a disk failure in
the bloom look-up.

A small test is added to diskrowset-test to ensure errors are returned
appropriately.

Change-Id: I41b56cbbf1b3b00fdb4dbf1ec08f12ced97088e7
---
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/diskrowset-test.cc
2 files changed, 37 insertions(+), 1 deletion(-)



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

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

[kudu-CR] [mini-cluster] allow 18-bit PID in LOOPBACK bind mode

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: [mini-cluster] allow 18-bit PID in LOOPBACK bind mode
..

[mini-cluster] allow 18-bit PID in LOOPBACK bind mode

This changelist allows to use 18-bit PID for binding
in LOOPBACK mode.  Prior to this patch, the minicluster supported
PIDs to be up to 16 bits wide.

While expanding the range for PIDs, this also narrows down the limit
on maximum number of 'indexed servers' to 62 (it was 262 prior to this
change).  Hopefully, that's enough for our minicluster tests.

The incentive to put up this patch was change of the max PID up to
147456 in /proc/sys/kernel/pid_max after recent restart of one
of our build servers.

Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130
---
M src/kudu/mini-cluster/mini_cluster.cc
1 file changed, 20 insertions(+), 7 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130
Gerrit-Change-Number: 8422
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8304 )

Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster
..


Patch Set 9: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8304/9/src/kudu/mini-cluster/external_mini_cluster-test.cc
File src/kudu/mini-cluster/external_mini_cluster-test.cc:

http://gerrit.cloudera.org:8080/#/c/8304/9/src/kudu/mini-cluster/external_mini_cluster-test.cc@28
PS9, Line 28: #include "kudu/gutil/strings/util.h" // IWYU pragma: keep
> HasPrefixString
And IWYU wants you to remove it due to the other changes in the file? Weird.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0
Gerrit-Change-Number: 8304
Gerrit-PatchSet: 9
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Oct 2017 23:08:12 +
Gerrit-HasComments: Yes

[kudu-CR] Add a benchmark test for tablet deletion

[Hao Hao (Code Review)]

Hao Hao has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8420


Change subject: Add a benchmark test for tablet deletion
..

Add a benchmark test for tablet deletion

Commit 15f3f9b2f optimizes the performance of group blocks deletion via
coalescing holes punch for log block manager. This patch adds a test to
benchmark tablet deletion for measuring actual improvement.

With 2.5k blocks on the tablet:

Before commit 15f3f9b2f:
I1030 15:25:38.700058 145431 tablet_server-test.cc:2336] Time spent deleting 
tablet: real 0.182s user 0.000s sys 0.000s
I1030 15:25:38.700096 145431 tablet_server-test.cc:2345] 
block_count_before_delete : 2500
I1030 15:25:38.700109 145431 tablet_server-test.cc:2346] 
log_block_manager_containers : 6
I1030 15:25:38.700124 145431 tablet_server-test.cc:2347] 
log_block_manager_holes_punched : 3000

After:
I1030 15:31:54.720243 24610 tablet_server-test.cc:2336] Time spent deleting 
tablet: real 0.062s user 0.001s sys 0.000s
I1030 15:31:54.720258 24610 tablet_server-test.cc:2345] 
block_count_before_delete : 2500
I1030 15:31:54.720268 24610 tablet_server-test.cc:2346] 
log_block_manager_containers : 6
I1030 15:31:54.720280 24610 tablet_server-test.cc:2347] 
log_block_manager_holes_punched : 638

Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
---
M src/kudu/tserver/tablet_server-test.cc
1 file changed, 66 insertions(+), 0 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao 

[kudu-CR] Add a benchmark test for tablet deletion

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8420 )

Change subject: Add a benchmark test for tablet deletion
..


Patch Set 2:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@131
PS1, Line 131: DEFINE_int32(delete_tablet_bench_num_flushes, 200,
 :  "Number of disk row sets to flush in the delete 
tablet benchmark");
 :
> Nit: the name and description of this new flag suggests that it's part of t
Done


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2293
PS1, Line 2293:   uint64_t rows = FLAGS_delete_tablet_bench_num_flushes;
> I don't think this is necessary; the other benchmark in this test isn't con
Done


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2295
PS1, Line 2295:   // Collect some related metrics.
  :   scoped_refptr block_count =
> How long does this take to run in slow mode? If it's less than 5s, I don't
With 'delete_tablet_bench_num_flushes' default to 200, the test ran for 3s. So 
I will remove it.


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2299
PS1, Line 2299:   scoped_refptr container =
> Nit: terminate with a period.
Done


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2300
PS1, Line 2300:   METRIC_log_block_manager_containers.Instantiate(
> Isn't this guaranteed to be true by the test harness itself? Perhaps it can
Done


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2303
PS1, Line 2303:   METRIC_log_block_manager_holes_punched.Instantiate(
  :   mini_server_->server()->metric_entity());
  :
  :   // Put some data in the tablet. We insert rows and flush 
immediately to
  :   // ensure that there is enough blocks on disk to run the 
benchmark.
  :   for (int i = 0; i < rows; i++) {
  : NO_FATALS(InsertTestRowsRemote(i, 1));
  : ASSERT_OK(tablet_replica_->tablet()->Flush());
  :   }
> I presume this is okay to execute even when block_manager=file?
Right, it is safe to do so.


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2314
PS1, Line 2314: ablet from within
> Nit: replace with "run the"
Done


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2316
PS1, Line 2316:   // by the test code.
> Nit: new code should use the shorter NO_FATALS() macro.
Done


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2335
PS1, Line 2335:   // Log the related metrics.
> No latency is being measured here.
Done


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2344
PS1, Line 2344:   // Verify that the tablet exists
> You can log this regardless of block manager, no?
Right, will remove the condition.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 31 Oct 2017 00:18:35 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2200: provide better diagnostics when connecting to a subset of masters

[Todd Lipcon (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: KUDU-2200: provide better diagnostics when connecting to a 
subset of masters
..

KUDU-2200: provide better diagnostics when connecting to a subset of masters

This changes the ConnectToMaster RPC to send back the list of the master
peers, and then changes the clients to use this information to provide
better error messages in the case that the user has specified only a
subset of the live masters.

Note that this patch follows a "first do no harm" philosophy. In the
case that the user "gets lucky" and only specifies one of their three
masters, and that master happens to be the leader, we'll continue to
treat that as "successful".

Change-Id: I52f903e1aa5ae6948ca1ba6d4d856c3c9dc73d56
---
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M 
java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java
M src/kudu/client/master_rpc.cc
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/master.h
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
10 files changed, 254 insertions(+), 9 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I52f903e1aa5ae6948ca1ba6d4d856c3c9dc73d56
Gerrit-Change-Number: 8393
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2200: provide better diagnostics when connecting to a subset of masters

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8393 )

Change subject: KUDU-2200: provide better diagnostics when connecting to a 
subset of masters
..


Patch Set 2:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8393/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java
File 
java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java:

http://gerrit.cloudera.org:8080/#/c/8393/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java@238
PS1, Line 238:
> pull this into a local variable.
Done


http://gerrit.cloudera.org:8080/#/c/8393/1/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java:

http://gerrit.cloudera.org:8080/#/c/8393/1/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@311
PS1, Line 311: PB pb : pbs) {
> simpler as 'new ArrayList<>(pbs.size());'
Done


http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.h
File src/kudu/master/master.h:

http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.h@94
PS1, Line 94:   //
> warning: missing username/bug in TODO [google-readability-todo]
Done


http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.cc
File src/kudu/master/master.cc:

http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.cc@335
PS1, Line 335: t
> whitespace
Done


http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.proto@616
PS1, Line 616:   //
> weirdly I found that in a non-replicated master setup the RaftConfigPB didn
looked into this more and it turns out that the RaftConfigPB does list the 
local peer but doesn't list its hostname/port, which I guess is on purpose so 
you could easily renumber/rename your master without confusing it.

Changed the code here to fill in the current hostname/port for this case.


http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.proto@620
PS1, Line 620: nly talk t
> 'Client implementations' or 'Clients'
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I52f903e1aa5ae6948ca1ba6d4d856c3c9dc73d56
Gerrit-Change-Number: 8393
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 31 Oct 2017 00:19:11 +
Gerrit-HasComments: Yes

[kudu-CR] Add a benchmark test for tablet deletion

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8420 )

Change subject: Add a benchmark test for tablet deletion
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc@2293
PS2, Line 2293:   uint64_t rows = FLAGS_delete_tablet_bench_num_flushes;
> Not needed anymore?
Done


http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc@2327
PS2, Line 2327: for
> Nit: remove this word
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 31 Oct 2017 00:34:33 +
Gerrit-HasComments: Yes

[kudu-CR] [java client] improve AsyncKuduScanner logging

[Hao Hao (Code Review)]

Hello Alexey Serbin, Dan Burkert, Jean-Daniel Cryans, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: [java client] improve AsyncKuduScanner logging
..

[java client] improve AsyncKuduScanner logging

As filed in KUDU-2199, in a jenkin's job build, ITClient test failed
even with fault tolerant scanner. The test log demonstrates that the
issue may cause by re-scan of scanned data. However, loop ITClient
1 times and did not discover any issues:
  http://dist-test.cloudera.org/job?job_id=hao.hao.1508869382.32617

This patch improves test coverage of ITClient so that error will be
reported if row count of each scan ever unexpectedly decrease. It also
improves logging of AsyncKuduScanner to give more information for
diagnosis if the same issue occur again. Another 2000 runs of ITClient
with current patch passed:
  http://dist-test.cloudera.org/job?job_id=hao.hao.1509045968.14317

Change-Id: I90ffcd01e7f99f3090fa118092fc303e06fb92dc
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
R 
java/kudu-client/src/main/java/org/apache/kudu/client/FaultTolerantScannerExpiredException.java
M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java
3 files changed, 22 insertions(+), 19 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I90ffcd01e7f99f3090fa118092fc303e06fb92dc
Gerrit-Change-Number: 8382
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a benchmark test for tablet deletion

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8420 )

Change subject: Add a benchmark test for tablet deletion
..


Patch Set 1:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@131
PS1, Line 131: DEFINE_int32(single_threaded_delete_tablet_bench_insert_rows, 
500,
 :  "Number of rows to insert in the testing phase of 
the single threaded "
 :  "tablet server delete tablet benchmark");
Nit: the name and description of this new flag suggests that it's part of the 
existing "insert latency micro-benchmark". Perhaps call it 
"delete_tablet_bench_num_flushes" and describe it with "Number of disk row sets 
to flush in the delete tablet benchmark"?


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2293
PS1, Line 2293: #ifdef NDEBUG
I don't think this is necessary; the other benchmark in this test isn't 
conditioned on RELEASE mode.


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2295
PS1, Line 2295:   uint64_t rows = AllowSlowTests() ?
  :   
FLAGS_single_threaded_delete_tablet_bench_insert_rows : 10;
How long does this take to run in slow mode? If it's less than 5s, I don't 
think it's worth the condition.


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2299
PS1, Line 2299:   // Verify that the tablet exists
Nit: terminate with a period.


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2300
PS1, Line 2300:   
ASSERT_TRUE(mini_server_->server()->tablet_manager()->LookupTablet(kTabletId, 
));
Isn't this guaranteed to be true by the test harness itself? Perhaps it can be 
omitted in favor of operating on tablet_replica_ directly?


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2303
PS1, Line 2303:   scoped_refptr block_count =
  :   
METRIC_log_block_manager_blocks_under_management.Instantiate(
  :   mini_server_->server()->metric_entity(), 0);
  :   scoped_refptr container =
  :   METRIC_log_block_manager_containers.Instantiate(
  :   mini_server_->server()->metric_entity(), 0);
  :   scoped_refptr holes_punched =
  :   METRIC_log_block_manager_holes_punched.Instantiate(
  :   mini_server_->server()->metric_entity());
I presume this is okay to execute even when block_manager=file?


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2314
PS1, Line 2314: do tablet deletion
Nit: replace with "run the"


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2316
PS1, Line 2316: ASSERT_NO_FATAL_FAILURE(InsertTestRowsRemote(i, 1));
Nit: new code should use the shorter NO_FATALS() macro.


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2335
PS1, Line 2335:   // Send the call and measure the latency.
No latency is being measured here.


http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2344
PS1, Line 2344:   if (FLAGS_block_manager == "log") {
You can log this regardless of block manager, no?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 30 Oct 2017 23:37:00 +
Gerrit-HasComments: Yes

[kudu-CR] [mini-cluster] allow 18-bit PID in LOOPBACK bind mode

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8422


Change subject: [mini-cluster] allow 18-bit PID in LOOPBACK bind mode
..

[mini-cluster] allow 18-bit PID in LOOPBACK bind mode

This changelist allows to use 18-bit PID for binding
in LOOPBACK mode.  Prior to this patch, the minicluster supported
PIDs to be up to 16 bits wide.

While expanding the range for PIDs, this also narrows down the limit
on maximum number of 'indexed daemons' to 62 (it was 262 prior to this
change).  Hopefully, that's enough for our minicluster tests.

The incentive to put up this patch was that I recently found one
of our build servers ending up with 147456 as pid_max
(/proc/sys/kernel/pid_max) after recent restart.

Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130
---
M src/kudu/mini-cluster/mini_cluster.cc
1 file changed, 12 insertions(+), 4 deletions(-)



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

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

[kudu-CR] [mini-cluster] allow 18-bit PID in LOOPBACK bind mode

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8422 )

Change subject: [mini-cluster] allow 18-bit PID in LOOPBACK bind mode
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8422/1/src/kudu/mini-cluster/mini_cluster.cc
File src/kudu/mini-cluster/mini_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/8422/1/src/kudu/mini-cluster/mini_cluster.cc@37
PS1, Line 37:   static const uint8_t INDEX_MAX = 0x3f; // 2^6 - 1
I think this function could probably be rewritten to be a bit clearer now that 
it's more complicated than it was before. The magic numbers here make it hard 
to follow. something along the lines of:

int kPidBits = 18;
int kServerBits = 24 - kPidBits;
CHECK_LT(index, 1 << kServerBits);
CHECK_LT(pid, 1 << kPidBits);
uint32_t ip = (pid << kServerBits) | kServer;
... extract octets from 'ip'



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130
Gerrit-Change-Number: 8422
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 31 Oct 2017 00:13:33 +
Gerrit-HasComments: Yes

[kudu-CR] Add a benchmark test for tablet deletion

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8420 )

Change subject: Add a benchmark test for tablet deletion
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc@2293
PS2, Line 2293:   uint64_t rows = FLAGS_delete_tablet_bench_num_flushes;
Not needed anymore?


http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc@2327
PS2, Line 2327: for
Nit: remove this word



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 31 Oct 2017 00:22:50 +
Gerrit-HasComments: Yes

[kudu-CR] mvcc: allow tablet shutdown without completing txs

[Andrew Wong (Code Review)]

Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: mvcc: allow tablet shutdown without completing txs
..

mvcc: allow tablet shutdown without completing txs

Currently, the only way to stop an Applying transaction is to wait for
it to finish and Commit it. This constraint was put in place to
guarantee on-disk correctness, but is sometimes too strict. E.g. if the
tablet is shutting down, the Apply doesn't need to finish.

This patch adds the ability to "stop" a tablet by closing its MVCC
manager. Once this happens, new Applies will return and not move to the
Commit phase, and any methods waiting for the tablet's Applies to Commit
(e.g. new snapshot scans, FlushMRS) will respond with an error
immediately. Applies that are already underway may still Commit, but
these Committed operations are inconsequential w.r.t. consistency;
having some in-flight transactions Commit and others not is consistent
with the server crashing in between the Commits of two transactions.
Additionally, once the MVCC manager is closed, new transactions will
abort immediately before even reaching the Prepare phase.

This will be particularly useful in handling disk failures: if a tablet
needs to be shut down due to disk failure, its MVCC manager can be
closed immediately, allowing currently-Applying transactions to abort
without Committing.

This patch only includes the behavior when shutting down a tablet, with
the assumption that a tablet will only be shut down when it's being
deleted and we don't care too much about its in-flight transactions
Committing. Code paths that previously crashed Kudu if Applies did not
succeed will now not crash if the MVCC manager is closed and log a
warning instead.

Testing is done by adding the following:
- a test in mvcc-test to shut down MVCC and delete an Applying
  transaction, ensuring that there are no errors when it leaves scope.
- a test in mvcc-test to wait on an Applying transaction, shut down
  MVCC, and ensure that any waiters will return with an error.
- a new test stop_tablet-itest is added to ensure stopped leaders don't
  complete writes and stopped followers do, and stopped tablets don't
  prevent fault-tolerant scans

Change-Id: I983620f27e7226806a2cca253db7619731914d42
---
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/stop_tablet-itest.cc
M src/kudu/tablet/mvcc-test.cc
M src/kudu/tablet/mvcc.cc
M src/kudu/tablet/mvcc.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica_mm_ops.cc
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tserver/tablet_service.cc
13 files changed, 505 insertions(+), 75 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 21
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] mvcc: allow tablet shutdown without completing txs

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7439 )

Change subject: mvcc: allow tablet shutdown without completing txs
..


Patch Set 21:

(26 comments)

http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc
File src/kudu/integration-tests/ts_tablet_manager-itest.cc:

http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@77
PS20, Line 77: using itest::SimpleIntKeyKuduSchema;
> warning: using decl 'InternalMiniCluster' is unused [misc-unused-using-decl
Done


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@130
PS20, Line 130:   ASSERT_OK(cluster->Start());
> this should probably be less specific to implementation details
Done


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@179
PS20, Line 179:   FLAGS_enable_leader_failure_detection = false;
> I'm not sure this test suite is the best place for these tests, since they
I moved to stop_tablet-itest.cc


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@183
PS20, Line 183:   FLAGS_allow_unsafe_replication_factor = true;
> here you're stopping a random server, not necessarily the one that the scan
Done


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@187
PS20, Line 187:
> are the readers using fault-tolerant scans by default here?
Yeah they are (see test_workload.cc:219)


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@197
PS20, Line 197:
> Somewhere we have a utility function like AssertNoCrashes(). Otherwise mayb
Done


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@205
PS20, Line 205:   ValueDeleter deleter(_map);
> in these tests it might be beneficial to configure the MM such that it cons
Done


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@235
PS20, Line 235:
> is it possible in this test to re-start the tserver that has the stopped ta
I think it'd be a bit tricky in this patch since this makes use of IMC instead 
of EMC, unless there's a ClusterVerifier equivalent for IMCs that I'm not aware 
of. Good point though, in EMC tests I've been verifying with ClusterVerifier.

Trickier still, this actually makes use of the fact that we're using IMC by 
stopping the tablet directly by reaching into the server, so I'd prefer to keep 
this test IMC.


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@238
PS20, Line 238:   // The MarkDirty() callback is on an async thread so it 
might take the
> can you combine this test with the above one, using a utility function that
Done


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc-test.cc
File src/kudu/tablet/mvcc-test.cc:

http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc-test.cc@498
PS20, Line 498: waiting_thread.join();
  :   ASSERT_OK(s);
> don't you want to join before assert here?
Done


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc-test.cc@598
PS20, Line 598:   waiting_thread.join();
  :   ASSERT_STR_CONTAINS(s.ToString(), "closed");
  :   ASSERT_TRUE(s.IsAborted());
  :
  :   // New waiters should abort immediately.
  :   s = mgr.WaitForApplyingTransactionsToCommit();
  :
> if you reordered the join and the ASSERT then you could avoid the complexit
Done


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h
File src/kudu/tablet/mvcc.h:

http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h@253
PS20, Line 253: Status::
> specify the type of status
Done


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h@254
PS20, Line 254:   Status WaitForApplyingTransactionsToCommit() const 
WARN_UNUSED_RESULT;
> worth adding a WARN_UNUSED_RESULT here
Done


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h@275
PS20, Line 275: not
> not?
Done


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h@280
PS20, Line 280: e bool i
> rename to is_closed since it's inlinable short method
Done


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:

http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h@160
PS20, Line 160:   //
> mind adding a WARN_UNUSED_RESULT to these functions that you're changing fr
Done


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h@405
PS20, Line 405:
> nit: "should" is kinda fuzzy. Do you mean "will"? Nicer for callers to thin
Done


http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h@405
PS20, Line 405:
  :  

[kudu-CR] KUDU-2200: provide better diagnostics when connecting to a subset of masters

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8393 )

Change subject: KUDU-2200: provide better diagnostics when connecting to a 
subset of masters
..


Patch Set 2: Code-Review+1

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8393/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java@108
PS1, Line 108: // One of the connections should have succeeded.
 : assertEquals(1, successes);
> I chatted with Dan about this on Slack last week. His opinion was that, if
Yah I'm just being kind of conservative.  If the client always hard fails when 
the number of masters doesn't match, then it may make it difficult to 
reconfigure and add/drop masters.  I admit it's something of a hypothetical 
concern, but I think this patch is going to get us most of the bang-for-buck as 
it is.


http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.cc
File src/kudu/master/master.cc:

http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.cc@344
PS1, Line 344:
> leaders are also VOTERs. This is a bit of a forward-looking if statement th
Woops, I actually looked up the enum values, but I think I confused Role with 
MemberType.


http://gerrit.cloudera.org:8080/#/c/8393/2/src/kudu/master/master.cc
File src/kudu/master/master.cc:

http://gerrit.cloudera.org:8080/#/c/8393/2/src/kudu/master/master.cc@352
PS2, Line 352:   if (!peer.has_last_known_addr()) {
Thanks.  This same issue had actually tripped me up in the HMS work (getting 
the master addrs to add to the HMS entry), so I'm glad to have a general 
solution available.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I52f903e1aa5ae6948ca1ba6d4d856c3c9dc73d56
Gerrit-Change-Number: 8393
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 31 Oct 2017 01:21:56 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7053 )

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Patch Set 29:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:

http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h@30
PS29, Line 30: #include "kudu/util/subprocess.h"
> You sure about that? I'm looking at https://stackoverflow.com/questions/601
Sorry for not responding before.  This appears to me to be toolchain dependent 
(probably libstdcxx vs libcxx), because this won't compile on macos (libcxx) 
with a forwards declare.  I'm not crazy about the idea of forgoing the 
simplicity of '= default;' just to satisfy IWYU, so I'd like to keep it as is.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 29
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Oct 2017 23:06:44 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8345 )

Change subject: KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager
..


Patch Set 1:

> Moving to an overall better lock implementation should reduce our cognitive 
> load here quite a bit - a good lock like absl::Mutex should have the same 
> performance as a spinlock but also be smart enough to go to sleep when 
> necessary instead of spinning.

Adar told me on slack: "@tlipcon I think we can merge it, I don't feel strongly 
that the `TSTabletManager` lock be a spinlock. Looks like there's an IWYU 
failure though."

So I'll rev this to fix the build issue


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I763abddd74d8b1dabb618318dc84256b533077e3
Gerrit-Change-Number: 8345
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 31 Oct 2017 00:22:13 +
Gerrit-HasComments: No

[kudu-CR] Add a benchmark test for tablet deletion

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8420 )

Change subject: Add a benchmark test for tablet deletion
..


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 31 Oct 2017 00:39:27 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8345 )

Change subject: KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I763abddd74d8b1dabb618318dc84256b533077e3
Gerrit-Change-Number: 8345
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 31 Oct 2017 00:39:07 +
Gerrit-HasComments: No

[kudu-CR] Add a macro form for ScopedCleanup

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8416 )

Change subject: Add a macro form for ScopedCleanup
..

Add a macro form for ScopedCleanup

This adds a new macro SCOPED_CLEANUP({ ... }) which is a shorter
form of 'auto cleanup = MakeScopedCleanup([&] { ... })

I also changed over a bunch of call sites to use it (those that were
easily discovered by a perl regex). We can switch over the remaining
ones as we run across them.

Change-Id: Idb72c27824def3c6a6d98c56b46cdd620f651e0e
Reviewed-on: http://gerrit.cloudera.org:8080/8416
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo 
---
M src/kudu/fs/log_block_manager.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/client-negotiation-failover-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/security-unknown-tsk-itest.cc
M src/kudu/integration-tests/tombstoned_voting-stress-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/security/ca/cert_management.cc
M src/kudu/security/init.cc
M src/kudu/security/openssl_util.cc
M src/kudu/util/env-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/net/net_util.cc
M src/kudu/util/scoped_cleanup-test.cc
M src/kudu/util/scoped_cleanup.h
M src/kudu/util/threadpool-test.cc
16 files changed, 53 insertions(+), 26 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Idb72c27824def3c6a6d98c56b46cdd620f651e0e
Gerrit-Change-Number: 8416
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] mvcc: allow tablet shutdown without completing txs

[Andrew Wong (Code Review)]

Andrew Wong has removed a vote on this change.

Change subject: mvcc: allow tablet shutdown without completing txs
..


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 21
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a benchmark test for tablet deletion

[Hao Hao (Code Review)]

Hello Kudu Jenkins, Adar Dembo,

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

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

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

Change subject: Add a benchmark test for tablet deletion
..

Add a benchmark test for tablet deletion

Commit 15f3f9b2f optimizes the performance of group blocks deletion via
coalescing holes punch for log block manager. This patch adds a test to
benchmark tablet deletion for measuring actual improvement.

With 2.5k blocks on the tablet:

Before commit 15f3f9b2f:
I1030 15:25:38.700058 145431 tablet_server-test.cc:2336] Time spent deleting 
tablet: real 0.182s user 0.000s sys 0.000s
I1030 15:25:38.700096 145431 tablet_server-test.cc:2345] 
block_count_before_delete : 2500
I1030 15:25:38.700109 145431 tablet_server-test.cc:2346] 
log_block_manager_containers : 6
I1030 15:25:38.700124 145431 tablet_server-test.cc:2347] 
log_block_manager_holes_punched : 3000

After:
I1030 15:31:54.720243 24610 tablet_server-test.cc:2336] Time spent deleting 
tablet: real 0.062s user 0.001s sys 0.000s
I1030 15:31:54.720258 24610 tablet_server-test.cc:2345] 
block_count_before_delete : 2500
I1030 15:31:54.720268 24610 tablet_server-test.cc:2346] 
log_block_manager_containers : 6
I1030 15:31:54.720280 24610 tablet_server-test.cc:2347] 
log_block_manager_holes_punched : 638

Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
---
M src/kudu/tserver/tablet_server-test.cc
1 file changed, 55 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager

[Todd Lipcon (Code Review)]

Hello David Ribeiro Alves, Kudu Jenkins, Adar Dembo,

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

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

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

Change subject: KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager
..

KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager

The TSTabletManager map is sometimes held for a long time. Given that, a
sleeping mutex is more appropriate than a spinlock.

Change-Id: I763abddd74d8b1dabb618318dc84256b533077e3
---
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
2 files changed, 25 insertions(+), 24 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I763abddd74d8b1dabb618318dc84256b533077e3
Gerrit-Change-Number: 8345
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [java client] improve AsyncKuduScanner logging

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8382 )

Change subject: [java client] improve AsyncKuduScanner logging
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8382/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java
File 
java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java:

http://gerrit.cloudera.org:8080/#/c/8382/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@727
PS3, Line 727: ret += ", lastPrimaryKey = " + 
Bytes.pretty(lastPrimaryKey);
> I'd lean on the side of not exposing it, since it's somewhat complicated to
Ok, then will remove it. As on the other hand, hashed lastPrimaryKey can only 
give very limited information.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I90ffcd01e7f99f3090fa118092fc303e06fb92dc
Gerrit-Change-Number: 8382
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Oct 2017 23:24:12 +
Gerrit-HasComments: Yes

[kudu-CR] Add a benchmark test for tablet deletion

[Hao Hao (Code Review)]

Hello Kudu Jenkins, Adar Dembo,

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

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

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

Change subject: Add a benchmark test for tablet deletion
..

Add a benchmark test for tablet deletion

Commit 15f3f9b2f optimizes the performance of group blocks deletion via
coalescing holes punch for log block manager. This patch adds a test to
benchmark tablet deletion for measuring actual improvement.

With 2.5k blocks on the tablet:

Before commit 15f3f9b2f:
I1030 15:25:38.700058 145431 tablet_server-test.cc:2336] Time spent deleting 
tablet: real 0.182s user 0.000s sys 0.000s
I1030 15:25:38.700096 145431 tablet_server-test.cc:2345] 
block_count_before_delete : 2500
I1030 15:25:38.700109 145431 tablet_server-test.cc:2346] 
log_block_manager_containers : 6
I1030 15:25:38.700124 145431 tablet_server-test.cc:2347] 
log_block_manager_holes_punched : 3000

After:
I1030 15:31:54.720243 24610 tablet_server-test.cc:2336] Time spent deleting 
tablet: real 0.062s user 0.001s sys 0.000s
I1030 15:31:54.720258 24610 tablet_server-test.cc:2345] 
block_count_before_delete : 2500
I1030 15:31:54.720268 24610 tablet_server-test.cc:2346] 
log_block_manager_containers : 6
I1030 15:31:54.720280 24610 tablet_server-test.cc:2347] 
log_block_manager_holes_punched : 638

Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
---
M src/kudu/tserver/tablet_server-test.cc
1 file changed, 53 insertions(+), 0 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf
Gerrit-Change-Number: 8420
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] mvcc: allow tablet shutdown without completing txs

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7439 )

Change subject: mvcc: allow tablet shutdown without completing txs
..


Patch Set 21:

...flake seemed unrelated... :)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42
Gerrit-Change-Number: 7439
Gerrit-PatchSet: 21
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Tue, 31 Oct 2017 02:31:44 +
Gerrit-HasComments: No

[kudu-CR] Add a macro form for ScopedCleanup

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8416 )

Change subject: Add a macro form for ScopedCleanup
..


Patch Set 1: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8416/1/src/kudu/util/scoped_cleanup.h
File src/kudu/util/scoped_cleanup.h:

http://gerrit.cloudera.org:8080/#/c/8416/1/src/kudu/util/scoped_cleanup.h@31
PS1, Line 31: // NOTE: in the case that you want to cancel the cleanup, use the 
more verbose
: // (non-macro) form below.
Worth mentioning that this form automatically captures everything by ref?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idb72c27824def3c6a6d98c56b46cdd620f651e0e
Gerrit-Change-Number: 8416
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 30 Oct 2017 21:13:25 +
Gerrit-HasComments: Yes

[kudu-CR] WIP: KUDU-2200: provide better diagnostics when connecting to a subset of masters

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8393 )

Change subject: WIP: KUDU-2200: provide better diagnostics when connecting to a 
subset of masters
..


Patch Set 1:

(4 comments)

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

http://gerrit.cloudera.org:8080/#/c/8393/1//COMMIT_MSG@9
PS1, Line 9: This changes the ConnectToMaster RPC to send back the list of the 
master
   : peers, and then changes the client to use this information to 
provide
   : better error messages in the case that the user has specified only 
a
   : subset of the live masters.
> How does this fit into the case of migration from single to multi-master se
The CLI tool during migration is using raw RPCs against the masters rather than 
the client, so I think it should be fine.


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

http://gerrit.cloudera.org:8080/#/c/8393/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java@108
PS1, Line 108: // One of the connections should have succeeded.
 : assertEquals(1, successes);
> I don't understand this.  I would expect all connections to fail because it
I chatted with Dan about this on Slack last week. His opinion was that, if the 
user happens to "guess right" and gets the leader master, we shouldn't 
proactively fail them. I think his thinking was "first do no harm" -- ie not 
cause any breakage in a situation that would otherwise have worked.

Dan, mind piping up with your reasoning here?


http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.cc
File src/kudu/master/master.cc:

http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.cc@344
PS1, Line 344: if (peer.member_type() == consensus::RaftPeerPB::VOTER) {
> Is filtering leader masters intentional?
leaders are also VOTERs. This is a bit of a forward-looking if statement that 
anticipates the possibility of non-voter leaders


http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.proto@616
PS1, Line 616:   // NOTE: This will not be filled in on a single-master cluster.
> +1
weirdly I found that in a non-replicated master setup the RaftConfigPB didn't 
have any masters. But I'll look again, maybe I did something wrong when I was 
trying this.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I52f903e1aa5ae6948ca1ba6d4d856c3c9dc73d56
Gerrit-Change-Number: 8393
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Oct 2017 21:41:20 +
Gerrit-HasComments: Yes

[kudu-CR] [webui] Add templates for tserver webui

[Will Berkeley (Code Review)]

Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon,

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

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

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

Change subject: [webui] Add templates for tserver webui
..

[webui] Add templates for tserver webui

This converts all the tablet server web ui endpoints to use a template,
unless it doesn't make sense to. This includes
- /scans
- /tablets
- /tablet
- /transactions
- /tablet-rowsetlayout-svg
- /tablet-consensus-status
- /log-anchors
- /dashboards

Change-Id: I99e08be9aa8abddd51ada61683b6f75190a00b5c
---
M src/kudu/server/webserver.cc
M src/kudu/server/webui_util.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tserver_path_handlers.cc
M src/kudu/tserver/tserver_path_handlers.h
A www/dashboards.mustache
A www/log-anchors.mustache
A www/scans.mustache
M www/table.mustache
A www/tablet-consensus-status.mustache
A www/tablet-rowsetlayout-svg.mustache
A www/tablet.mustache
A www/tablets.mustache
A www/transactions.mustache
14 files changed, 604 insertions(+), 384 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I99e08be9aa8abddd51ada61683b6f75190a00b5c
Gerrit-Change-Number: 8307
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2191 (2/n): Hive Metastore client

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7053 )

Change subject: KUDU-2191 (2/n): Hive Metastore client
..


Patch Set 36:

OK I think this is ready to review again.  Sorry for the churn.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee
Gerrit-Change-Number: 7053
Gerrit-PatchSet: 36
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Oct 2017 18:40:18 +
Gerrit-HasComments: No

[kudu-CR] mini-cluster: support parallel multi-master clusters

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8280 )

Change subject: mini-cluster: support parallel multi-master clusters
..


Patch Set 2:

I got some time to bang on this on a recent flight, but I still don't think 
it's in a comittable state.  I recall running into an issue with reuse port, 
but I don't remember what (I recall thinking I'd need a linux test box, and had 
no internet access).  Anyway, I'll pick this back up when I have more time


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c
Gerrit-Change-Number: 8280
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 30 Oct 2017 18:47:52 +
Gerrit-HasComments: No

[kudu-CR] mini-cluster: support parallel multi-master clusters

[Dan Burkert (Code Review)]

Hello Tidy Bot, Kudu Jenkins, Adar Dembo,

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

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

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

Change subject: mini-cluster: support parallel multi-master clusters
..

mini-cluster: support parallel multi-master clusters

This commit refactors the mini-clusters to internally use reserved
sockets for their child daemons. Reserved sockets are simply sockets
bound to a random port with SO_REUSEPORT. This allows for a few
simplifications:

* master addresses in multi-master clusters can be reserved up-front,
  which removes any chance of a race condition in binding. As a result,
  multi-master clusters no longer need hard-coded ports. As a result,
  tests using multi-master clusters no longer need to run in sequence.
* external mini cluster tablet servers also now use a reserved port, so
  that when the tablet server processes is restarted there is no chance
  of a port conflict. As a result, we no longer are required to use the
  'unique loopback' hack on Linux. Internal mini cluster tablet servers
  still bind to a non-reserved port, since we never restart internal
  mini-cluster tablet servers.

Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c
---
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/catalog_manager_tsk-itest.cc
M src/kudu/integration-tests/client-negotiation-failover-itest.cc
M src/kudu/integration-tests/client-stress-test.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/integration-tests/master_migration-itest.cc
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/integration-tests/security-faults-itest.cc
M src/kudu/integration-tests/token_signer-itest.cc
M src/kudu/integration-tests/webserver-stress-itest.cc
M src/kudu/master/mini_master.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/mini-cluster/internal_mini_cluster.cc
M src/kudu/mini-cluster/internal_mini_cluster.h
M src/kudu/mini-cluster/mini_cluster.cc
M src/kudu/mini-cluster/mini_cluster.h
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/ksck_remote-test.cc
M src/kudu/tools/tool_action_test.cc
25 files changed, 162 insertions(+), 236 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c
Gerrit-Change-Number: 8280
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] [build] fix OPENSSL ROOT DIR override on RH/CentOS

[Alexey Serbin (Code Review)]

Alexey Serbin has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8407 )

Change subject: [build] fix OPENSSL_ROOT_DIR override on RH/CentOS
..

[build] fix OPENSSL_ROOT_DIR override on RH/CentOS

Due to an extra level of indirection in the condition for one if()
expression, the OpenSSL location was not overridden on RedHat/CentOS
machines where the OpenSSL workaround build existed in
$KUDU_ROOT/thirdparty/installed/openssl-el6-workaround,
even if the OPENSSL_ROOT_DIR was overridden for cmake invocation
(i.e. -DOPENSSL_ROOT_DIR=/path/to/alt-openssl-location was specified).

Removing the level of indirection for the OPENSSL_ROOT_DIR variable
in the expression fixed the issue, which seems to be in sync with
  https://cmake.org/cmake/help/v3.9/command/if.html

At least, with this patch the issue is gone if running cmake 3.9.0.

Change-Id: Ibe69b5d62ef4054e60138a154a85569d92166aff
Reviewed-on: http://gerrit.cloudera.org:8080/8407
Tested-by: Alexey Serbin 
Reviewed-by: Todd Lipcon 
---
M CMakeLists.txt
1 file changed, 1 insertion(+), 2 deletions(-)

Approvals:
  Alexey Serbin: Verified
  Todd Lipcon: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibe69b5d62ef4054e60138a154a85569d92166aff
Gerrit-Change-Number: 8407
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] mini-cluster: support parallel multi-master clusters

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8280 )

Change subject: mini-cluster: support parallel multi-master clusters
..


Patch Set 2:

It's coming back to me now; at least one of the issues is that we rely on lsof 
to tell us if ports are bound / a service is ready, and the reuse_port trick 
messes with that.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c
Gerrit-Change-Number: 8280
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 30 Oct 2017 18:55:03 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2205. Improve error message for failed globs

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8415 )

Change subject: KUDU-2205. Improve error message for failed globs
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7e487b5f6b24c2a2bd66e33f5b51a31e6585657
Gerrit-Change-Number: 8415
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Mon, 30 Oct 2017 19:36:10 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster

[Dan Burkert (Code Review)]

Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,

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

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

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

Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster
..

KUDU-2191 (3/n): Add mini HMS option to external mini cluster

A couple of features are explicitly being punted on for now:

- If kerberos and the HMS are both configured, the HMS should be
  kerberized as well. This will come in a follow-up commit (the
  current HMS C++ client can't be used against a kerberized HMS).
- No API is provided in the mini-cluster tool to retrieve the HMS
  address. This will be added if/when it becomes necessary.

Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0
---
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action_test.cc
6 files changed, 73 insertions(+), 8 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0
Gerrit-Change-Number: 8304
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8304 )

Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster
..


Patch Set 6: -Verified

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8304/6/src/kudu/mini-cluster/external_mini_cluster.h
File src/kudu/mini-cluster/external_mini_cluster.h:

http://gerrit.cloudera.org:8080/#/c/8304/6/src/kudu/mini-cluster/external_mini_cluster.h@138
PS6, Line 138:   // If true, set up a Hive Metastore as part of this 
ExternalMiniCluster.
> include a line Default: false/true
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0
Gerrit-Change-Number: 8304
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Oct 2017 17:37:50 +
Gerrit-HasComments: Yes

[kudu-CR] Update auth token validity seconds description

[Dan Burkert (Code Review)]

Hello Alexey Serbin,

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

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

to review the following change.


Change subject: Update auth_token_validity_seconds description
..

Update auth_token_validity_seconds description

The caveats are no longer relevant since KUDU-2013 landed.

Change-Id: I050e39d0377049cdaac0afb5085a8a9a19d620a5
---
M src/kudu/master/master.cc
1 file changed, 2 insertions(+), 3 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I050e39d0377049cdaac0afb5085a8a9a19d620a5
Gerrit-Change-Number: 8413
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Alexey Serbin 

[kudu-CR] [build] fix OPENSSL ROOT DIR override on RH/CentOS

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8407 )

Change subject: [build] fix OPENSSL_ROOT_DIR override on RH/CentOS
..


Patch Set 5: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe69b5d62ef4054e60138a154a85569d92166aff
Gerrit-Change-Number: 8407
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Oct 2017 18:45:24 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2205. Improve error message for failed globs

[Todd Lipcon (Code Review)]

Hello Will Berkeley,

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

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

to review the following change.


Change subject: KUDU-2205. Improve error message for failed globs
..

KUDU-2205. Improve error message for failed globs

This fixes the Env::Glob function to give a more understandable error
message on typical issues such as permission denied. This can improve
the diagnosability in the case that a server is started with an
inaccessible log directory.

Change-Id: Ib7e487b5f6b24c2a2bd66e33f5b51a31e6585657
---
M src/kudu/util/env-test.cc
M src/kudu/util/env_posix.cc
2 files changed, 21 insertions(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib7e487b5f6b24c2a2bd66e33f5b51a31e6585657
Gerrit-Change-Number: 8415
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster

[Dan Burkert (Code Review)]

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon,

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

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

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

Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster
..

KUDU-2191 (3/n): Add mini HMS option to external mini cluster

A couple of features are explicitly being punted on for now:

- If kerberos and the HMS are both configured, the HMS should be
  kerberized as well. This will come in a follow-up commit (the
  current HMS C++ client can't be used against a kerberized HMS).
- No API is provided in the mini-cluster tool to retrieve the HMS
  address. This will be added if/when it becomes necessary.

Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0
---
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action_test.cc
6 files changed, 74 insertions(+), 9 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0
Gerrit-Change-Number: 8304
Gerrit-PatchSet: 8
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster

[Dan Burkert (Code Review)]

Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon,

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

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

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

Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster
..

KUDU-2191 (3/n): Add mini HMS option to external mini cluster

A couple of features are explicitly being punted on for now:

- If kerberos and the HMS are both configured, the HMS should be
  kerberized as well. This will come in a follow-up commit (the
  current HMS C++ client can't be used against a kerberized HMS).
- No API is provided in the mini-cluster tool to retrieve the HMS
  address. This will be added if/when it becomes necessary.

Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0
---
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
M src/kudu/tools/tool.proto
M src/kudu/tools/tool_action_test.cc
6 files changed, 79 insertions(+), 10 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0
Gerrit-Change-Number: 8304
Gerrit-PatchSet: 9
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8304 )

Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster
..


Patch Set 9:

This is ready for review again.  Sorry for the churn.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0
Gerrit-Change-Number: 8304
Gerrit-PatchSet: 9
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Mon, 30 Oct 2017 19:01:29 +
Gerrit-HasComments: No