[kudu-CR] KUDU-1704: add READ YOUR WRITES scan mode

[Hao Hao (Code Review)]

Hello Alexey Serbin, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Andrew Wong,

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

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

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

Change subject: KUDU-1704: add READ_YOUR_WRITES scan mode
..

KUDU-1704: add READ_YOUR_WRITES scan mode

This patch adds a new read mode READ_YOUR_WRITES on tserver. In this mode,
the server will pick a snapshot in the past, subject to the propagated
timestamp bound, and perform a read. Moreover, the chosen snapshot
timestamp is returned back to the client.

The major difference between READ_AT_SNAPSHOT scan without a timestamp
and READ_YOUR_WRITES scan is the latter will choose the newest timestamp
within the staleness bound that allows execution of the reads without
being blocked by the in-flight transactions. READ_OWN_WRITES mode is not
repeatable. However, it allows client local read-your-writes.

Design doc: 
https://docs.google.com/document/d/1WRLzKdCmRxXjUpi-DZsz7l2gl6215BsNdORtDXrmgl0/edit?usp=sharing

Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
---
M src/kudu/common/common.proto
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
4 files changed, 260 insertions(+), 52 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] KUDU-1704: add c++ client support for READ YOUR WRITES mode

[Hao Hao (Code Review)]

Hello David Ribeiro Alves, Kudu Jenkins,

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

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

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

Change subject: KUDU-1704: add c++ client support for READ_YOUR_WRITES mode
..

KUDU-1704: add c++ client support for READ_YOUR_WRITES mode

Change-Id: I34214245a78aed172a28fbdb395ff5bccd0fc0e1
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/scanner-internal.cc
6 files changed, 159 insertions(+), 61 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I34214245a78aed172a28fbdb395ff5bccd0fc0e1
Gerrit-Change-Number: 8823
Gerrit-PatchSet: 4
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

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

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

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

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
32 files changed, 1,339 insertions(+), 171 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 22
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2279 (part 5): don't include entity attributes in log

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Mike Percy,

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

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

to review the following change.


Change subject: KUDU-2279 (part 5): don't include entity attributes in log
..

KUDU-2279 (part 5): don't include entity attributes in log

Entity attributes like table names and partition descriptions aren't
very useful in the context of the metrics log. They can always be
grabbed from other places like 'kudu remote_replica list', etc, and are
rarely relevant for tserver-scope debugging.

Additionally, these might be considered somewhat sensitive for users to
share publically in bug reports, etc, so may as well "redact" them.

This shaves a few more bytes off the entries in the metrics log.

Change-Id: Ic995f6a3b8430b73a041de8e5bcbc53a5d8e7f1b
---
M src/kudu/server/server_base.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
3 files changed, 15 insertions(+), 6 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic995f6a3b8430b73a041de8e5bcbc53a5d8e7f1b
Gerrit-Change-Number: 9180
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] jsonwriter: do some internal buffering in front of ostringstream

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Mike Percy,

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

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

to review the following change.


Change subject: jsonwriter: do some internal buffering in front of ostringstream
..

jsonwriter: do some internal buffering in front of ostringstream

JsonWriter outputs into a std::ostringstream, and the RapidJSON
interface is such that we always append one character at a time.
ostringstream::put(char) is not inlined, so this is very slow.

This patch adds a small buffer in front of the ostringstream. We flush
the buffer on the end of any JSON array or object.

The patch adds a simple benchmark to show the improvement:

Before:
I0131 20:26:09.437477 16201 jsonwriter-test.cc:187] Throughput: 87.0644MB/sec

After:
I0131 20:24:17.889117 15929 jsonwriter-test.cc:187] Throughput: 154.192MB/sec

The remaining perf issues seem to be due to our use of protobuf
reflection.

Change-Id: Ib45d58187cf80ecfe2f5a25e40d7042e4a27619d
---
M src/kudu/util/jsonwriter-test.cc
M src/kudu/util/jsonwriter.cc
2 files changed, 73 insertions(+), 21 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib45d58187cf80ecfe2f5a25e40d7042e4a27619d
Gerrit-Change-Number: 9181
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Mike Percy,

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

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

to review the following change.


Change subject: KUDU-2279 (part 2): metrics: only emit changed metrics in 
metrics log
..

KUDU-2279 (part 2): metrics: only emit changed metrics in metrics log

This adds a global 'metrics epoch' which can be externally incremented.
When metrics are modified, they remember the epoch of their most recent
modification.

When we dump metrics, we can pass a lower bound in order to see only
metrics which have been modified in or after a given epoch.

This patch updates the metrics logging to only emit metrics that have
changed in each successive line. This should substantially reduce the
size and CPU cost of metric logging on servers with thousands of
tablets.

Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
---
M src/kudu/server/server_base.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
4 files changed, 106 insertions(+), 7 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia26be99a1fa96d52e2ca0905844d56c096d3778e
Gerrit-Change-Number: 9176
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2279 (part 4): enable metrics log by default

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Mike Percy,

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

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

to review the following change.


Change subject: KUDU-2279 (part 4): enable metrics log by default
..

KUDU-2279 (part 4): enable metrics log by default

Change-Id: Iaeb4c546c2800d8f4288a3626b9e998cd7e35e5c
---
M src/kudu/server/server_base_options.cc
1 file changed, 1 insertion(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaeb4c546c2800d8f4288a3626b9e998cd7e35e5c
Gerrit-Change-Number: 9178
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2279 (part 1). rolling log: respect logfile retention flag

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Mike Percy,

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

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

to review the following change.


Change subject: KUDU-2279 (part 1). rolling_log: respect logfile retention flag
..

KUDU-2279 (part 1). rolling_log: respect logfile retention flag

Previously we never deleted old metrics logs, making it somewhat
dangerous to enable this in production. Now we respect the same
retention gflag that we apply to our other glogs.

Change-Id: If3cc9b085c9827fd544b0bf7d7ae868e3692296b
---
M src/kudu/util/rolling_log-test.cc
M src/kudu/util/rolling_log.cc
M src/kudu/util/rolling_log.h
3 files changed, 81 insertions(+), 38 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If3cc9b085c9827fd544b0bf7d7ae868e3692296b
Gerrit-Change-Number: 9175
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2279 (part 3): metrics: don't emit untouched metrics to log

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Mike Percy,

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

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

to review the following change.


Change subject: KUDU-2279 (part 3): metrics: don't emit untouched metrics to log
..

KUDU-2279 (part 3): metrics: don't emit untouched metrics to log

This avoids a big dump of zero-valued metrics at startup.

Change-Id: I92d2c4640e54c91791fab9c420215bafa3fe8f20
---
M src/kudu/server/server_base.cc
M src/kudu/util/metrics-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
4 files changed, 69 insertions(+), 3 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I92d2c4640e54c91791fab9c420215bafa3fe8f20
Gerrit-Change-Number: 9177
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] metrics: fast-path PB/JSON dump for empty histograms

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Mike Percy,

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

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

to review the following change.


Change subject: metrics: fast-path PB/JSON dump for empty histograms
..

metrics: fast-path PB/JSON dump for empty histograms

It turns out that a decent percentage of our histograms are empty in
real life use cases. For example, some tablets might be cold enough that
they have never had any action since the tserver started. Other
histograms might correspond to features which aren't being used in a
particular workload (eg commit-wait counters).

For example, in a test cluster running a cycling YCSB workload, about
20% of histograms are empty. In another test cluster running more of an
analytic workload with a lot of very cold tablets, 81% of histograms
were empty.

We can fast-path various calculations on these histograms to just return
0 for a simple speedup on the /metricsjson endpoint.

Change-Id: I0732e8299e2f27c7c6021ec70740984ab46a69c0
---
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
2 files changed, 37 insertions(+), 21 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0732e8299e2f27c7c6021ec70740984ab46a69c0
Gerrit-Change-Number: 9179
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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

Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:

http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc@48
PS5, Line 48: extern const size_t kSaslMaxOutBufLen = 64 * 1024;
> hrm, so I guess we need to either set this max buf len to be quite large (1
I figured out a workaround, which is pretty much to do exactly what Thrift 
does: completely ignore the max buffer length.  I've set it to 100MiB to match 
the max for thrift and the HMS, hopefully that's big enough.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 01 Feb 2018 01:51:17 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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

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

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

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

Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
..

KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

The bulk of this commit is adding a new Thrift transport type,
SaslClientTransport, which facilitates SASL GSSAPI negotiation, as well
as integrity/privacy channel protection. The new transport is based on
Impala's version with some significant changes:

- Impala has a client and server SASL transport, necessitating a common
  superclass (SaslTransport). Since we only need a client transport, I
  collapsed all of the logic into a single class, which I think makes the
  code easier to follow.
- The transport uses Kudu helper types where possible, e.g., faststring
  buffers, and our existing SASL utility infrastructure.
- Integrity and privacy channel protection are implemented.

There are no standlone unit-tests for the transport, since that would
require implementing the server-specific counterpart. Instead, the class
is tested indirectly through using the HMS client to communicate with a
Kerberos-enabled HMS instance.

Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
---
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
A src/kudu/hms/sasl_client_transport.cc
A src/kudu/hms/sasl_client_transport.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
14 files changed, 953 insertions(+), 77 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] maintenance manager: log the reason for scheduling each operation

[Jean-Daniel Cryans (Code Review)]

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

Change subject: maintenance_manager: log the reason for scheduling each 
operation
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9172/1/src/kudu/util/maintenance_manager.cc@444
PS1, Line 444: best performance improvement
> Any suggestions what to use instead? We already use "perf improvement" on t
It's not just this log that worries me but it's the one that worries me the 
most. Maybe just making them less verbose:
log gc (x bytes)
memory pressure (x bytes)
data gc (x bytes)
perf improvement (score=blah)

But good point about perf improvement being already used in the UI.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4dcdb863a7a0b0fc2a72757801d5c057fa725c34
Gerrit-Change-Number: 9172
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Wed, 31 Jan 2018 23:43:49 +
Gerrit-HasComments: Yes

[kudu-CR] maintenance manager: log the reason for scheduling each operation

[Todd Lipcon (Code Review)]

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

Change subject: maintenance_manager: log the reason for scheduling each 
operation
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9172/1/src/kudu/util/maintenance_manager.cc@444
PS1, Line 444: best performance improvement
> I'm worried what random users might read into this kind of log line.
Any suggestions what to use instead? We already use "perf improvement" on the 
MM web UI.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4dcdb863a7a0b0fc2a72757801d5c057fa725c34
Gerrit-Change-Number: 9172
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Wed, 31 Jan 2018 23:31:51 +
Gerrit-HasComments: Yes

[kudu-CR] maintenance manager: log the reason for scheduling each operation

[Jean-Daniel Cryans (Code Review)]

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

Change subject: maintenance_manager: log the reason for scheduling each 
operation
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9172/1/src/kudu/util/maintenance_manager.cc@444
PS1, Line 444: best performance improvement
I'm worried what random users might read into this kind of log line.

Also, would it make sense to print something related to the priority order 
instead of having to lookup the code? Maybe we'd still have to look at the code 
to see what the priority means. Just a thought.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4dcdb863a7a0b0fc2a72757801d5c057fa725c34
Gerrit-Change-Number: 9172
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Wed, 31 Jan 2018 23:23:05 +
Gerrit-HasComments: Yes

[kudu-CR] maintenance manager: log the reason for scheduling each operation

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Jean-Daniel Cryans,

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

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

to review the following change.


Change subject: maintenance_manager: log the reason for scheduling each 
operation
..

maintenance_manager: log the reason for scheduling each operation

This makes it easier to troubleshoot when the maintenance manager
appears to be scheduling the "wrong" operation.

Example output from a long run of full_stack_insert_scan-test:

I0131 15:00:21.154744 13887 maintenance_manager.cc:300] P 
e96b0ba97e104a4294feef1163f7383f: Scheduling 
FlushMRSOp(eaa9b5237a14425e852ca97c2b4ae138): under memory pressure (60.52% 
used), running op which anchors most memory (642916119 bytes)
I0131 15:00:24.241927 13887 maintenance_manager.cc:300] P 
e96b0ba97e104a4294feef1163f7383f: Scheduling 
LogGCOp(eaa9b5237a14425e852ca97c2b4ae138): can GC 394382000 bytes of logs with 
low IO cost
I0131 15:00:24.309976 13887 maintenance_manager.cc:300] P 
e96b0ba97e104a4294feef1163f7383f: Scheduling 
UndoDeltaBlockGCOp(eaa9b5237a14425e852ca97c2b4ae138): can free up the most data 
on disk (29912621 bytes)
I0131 15:00:24.310154 13887 maintenance_manager.cc:300] P 
e96b0ba97e104a4294feef1163f7383f: Scheduling 
CompactRowSetsOp(eaa9b5237a14425e852ca97c2b4ae138): best performance 
improvement (score=0.281733)
I0131 15:00:26.482787 13887 maintenance_manager.cc:300] P 
e96b0ba97e104a4294feef1163f7383f: Scheduling 
CompactRowSetsOp(eaa9b5237a14425e852ca97c2b4ae138): best performance 
improvement (score=0.281354)
I0131 15:00:28.514597 13887 maintenance_manager.cc:300] P 
e96b0ba97e104a4294feef1163f7383f: Scheduling 
UndoDeltaBlockGCOp(eaa9b5237a14425e852ca97c2b4ae138): can free up the most data 
on disk (36234163 bytes)
I0131 15:00:28.514787 13887 maintenance_manager.cc:300] P 
e96b0ba97e104a4294feef1163f7383f: Scheduling 
CompactRowSetsOp(eaa9b5237a14425e852ca97c2b4ae138): best performance 
improvement (score=0.204581)
I0131 15:00:30.165297 13887 maintenance_manager.cc:300] P 
e96b0ba97e104a4294feef1163f7383f: Scheduling 
CompactRowSetsOp(eaa9b5237a14425e852ca97c2b4ae138): best performance 
improvement (score=0.150718)
I0131 15:00:31.783936 13887 maintenance_manager.cc:300] P 
e96b0ba97e104a4294feef1163f7383f: Scheduling 
CompactRowSetsOp(eaa9b5237a14425e852ca97c2b4ae138): best performance 
improvement (score=0.024950)
I0131 15:00:32.740442 13887 maintenance_manager.cc:300] P 
e96b0ba97e104a4294feef1163f7383f: Scheduling 
UndoDeltaBlockGCOp(eaa9b5237a14425e852ca97c2b4ae138): can free up the most data 
on disk (38786747 bytes)
I0131 15:00:36.495453 13887 maintenance_manager.cc:300] P 
e96b0ba97e104a4294feef1163f7383f: Scheduling 
FlushMRSOp(eaa9b5237a14425e852ca97c2b4ae138): under memory pressure (60.45% 
used), running op which anchors most memory (637714199 bytes)

Change-Id: I4dcdb863a7a0b0fc2a72757801d5c057fa725c34
---
M src/kudu/util/debug/trace_logging.h
M src/kudu/util/maintenance_manager-test.cc
M src/kudu/util/maintenance_manager.cc
M src/kudu/util/maintenance_manager.h
4 files changed, 74 insertions(+), 50 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4dcdb863a7a0b0fc2a72757801d5c057fa725c34
Gerrit-Change-Number: 9172
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] internal mini cluster: support Cluster/LogVerifier

[Andrew Wong (Code Review)]

Hello Tidy Bot, Mike Percy, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: internal_mini_cluster: support Cluster/LogVerifier
..

internal_mini_cluster: support Cluster/LogVerifier

This patch encapsulates MiniClusterFsInspector into an abstract class,
subclasses it as the existing external cluster inspector, and introduces
a new subclass for inspecting internal clusters. With these classes, the
LogVerifier, and thus, the ClusterVerifier can support both External-
and InternalMiniClusters.

The LogVerifier would originally open a read-only FsManager (which in
turn would open a BlockManager and a DataDirManager) per server and pass
it to the LogReader to inspect the WALs. Instead, the LogVerifier now
creates a MiniClusterFsInspector, which is more lightweight and defined
per MiniCluster. To the LogReader, it passes the WAL directory and an
env, which is all the LogReader needed from the FsManager in the first
place.

To test, I updated a test case in ts_tablet_manager-itest to make use of
the new ClusterVerifier with an internal cluster. CheckCluster() uses a
LogVerifier, which in this case uses an InternalMiniClusterFsInspector.

Change-Id: I228a6e3ba1a42db4e243ffdc5116f0c60ee04a84
---
M src/kudu/consensus/log-test.cc
M src/kudu/consensus/log_reader.cc
M src/kudu/consensus/log_reader.h
M src/kudu/fs/fs_manager.h
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/cluster_verifier.cc
D src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.h
A src/kudu/integration-tests/internal_mini_cluster_fs_inspector.h
M src/kudu/integration-tests/log_verifier.cc
M src/kudu/integration-tests/log_verifier.h
A src/kudu/integration-tests/mini_cluster_fs_inspector.cc
A src/kudu/integration-tests/mini_cluster_fs_inspector.h
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/tablet/tablet_bootstrap.cc
16 files changed, 698 insertions(+), 628 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I228a6e3ba1a42db4e243ffdc5116f0c60ee04a84
Gerrit-Change-Number: 9137
Gerrit-PatchSet: 9
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [thirdparty] fix sized-deallocation issue on OS X 10.11

[Alexey Serbin (Code Review)]

Hello Dan Burkert, Todd Lipcon,

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

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

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

Change subject: [thirdparty] fix sized-deallocation issue on OS X 10.11
..

[thirdparty] fix sized-deallocation issue on OS X 10.11

Fixed the issue of building gperftools 2.6.3 on OS X 10.11.  Also,
updated the top-level CMakeLists.txt to enable or disable the sized
deallocation feature for Kudu binaries accordingly.

Change-Id: I12632df70137bf5aed8b44d613b08856a925d840
---
M CMakeLists.txt
M thirdparty/download-thirdparty.sh
A thirdparty/patches/gperftools-sized-alloc-build-fix.patch
3 files changed, 49 insertions(+), 7 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I12632df70137bf5aed8b44d613b08856a925d840
Gerrit-Change-Number: 9159
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [thirdparty] fix sized-deallocation issue on OS X 10.11

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9159 )

Change subject: [thirdparty] fix sized-deallocation issue on OS X 10.11
..


Patch Set 1:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/9159/1/CMakeLists.txt@1093
PS1, Line 1093:   execute_process(
> is this now redundant with the above compiler version checks? ie if this su
Indeed.  That's a good observation!  Done.


http://gerrit.cloudera.org:8080/#/c/9159/1/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:

http://gerrit.cloudera.org:8080/#/c/9159/1/thirdparty/download-thirdparty.sh@134
PS1, Line 134:   patch -p1 < 
$TP_DIR/patches/gperftools-sized-alloc-build-fix.patch
> did you file a bug upstream with gperftools for this?
I filed the following issue at gperftools github repo: 
https://github.com/gperftools/gperftools/issues/954



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I12632df70137bf5aed8b44d613b08856a925d840
Gerrit-Change-Number: 9159
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 31 Jan 2018 21:15:39 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Todd Lipcon (Code Review)]

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

Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc
File src/kudu/rpc/sasl_common.cc:

http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/rpc/sasl_common.cc@48
PS5, Line 48: extern const size_t kSaslMaxOutBufLen = 64 * 1024;
> Yes, unfortunately right now we'll fail to receive large objects sent by th
hrm, so I guess we need to either set this max buf len to be quite large (1gb 
or something) or we need to fix the Java bug and make sure that HMS upstream 
uses a bug-fixed thrift?

I am a somewhat-inactive Thrift committer so the first half of that shouldn't 
be too hard to get done, happy to review and commit a fix



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 31 Jan 2018 21:11:04 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling

[Todd Lipcon (Code Review)]

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

Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
..

KUDU-2191 (4/n): HMS Thrift client fault handling

This commit improves the new HMS Thrift client's ability to handle
faults. In particular:

- The client now uses send, receive, and connect timeouts so that a
  non-responsive HMS instance will not block the client indefinitely.
- The Thrift logging callback is hooked up to glog so that we get proper
  log messages from Thrift.
- The HmsClient class and method docs include information about behavior
  when errors are encountered.

In the part 2 review, Todd also brought up the prospect of creating a
wrapper Thrift socket or transport to inject slow log warning messages
automatically. I've held off doing this for now, because I haven't been
able to figure out a way to do that which can associate the slowness
with higher-level operations like 'create database', as opposed to
lower-level like 'socket write'. I've made sure to apply the slow
warning calls uniformly across the HmsClient methods, and I don't think
it will be too onerous to keep them consistent in the future.

Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Reviewed-on: http://gerrit.cloudera.org:8080/8494
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
6 files changed, 191 insertions(+), 23 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 12
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 (4/n): HMS Thrift client fault handling

[Todd Lipcon (Code Review)]

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

Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
..


Patch Set 11: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 11
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: Wed, 31 Jan 2018 21:02:45 +
Gerrit-HasComments: No

[kudu-CR] KUDU-1613: evict replicas with wrong server UUIDs

[Andrew Wong (Code Review)]

Andrew Wong has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9136 )

Change subject: KUDU-1613: evict replicas with wrong server UUIDs
..

KUDU-1613: evict replicas with wrong server UUIDs

Currently, if a tablet server is wiped and restarted at the same host
and port with a new UUID assigned to it before the replicas on its
previous incarnation are evicted, those replicas won't ever be evicted.
With this patch, upon receiving a response with WRONG_SERVER_UUID,
leaders will treat the follower as failed and handle it appropriately,
re-replicating them elsewhere.

A new test case is added to raft_consensus-itest that is set up to
prompt eviction (i.e. low timeouts, ample servers to replicate to), and
without the change, this test fails.

Change-Id: If0c7ae9dfbd99c2c9a38e8b1f7748c70882e06b4
Reviewed-on: http://gerrit.cloudera.org:8080/9136
Reviewed-by: David Ribeiro Alves 
Tested-by: Kudu Jenkins
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.h
3 files changed, 95 insertions(+), 7 deletions(-)

Approvals:
  David Ribeiro Alves: Looks good to me, approved
  Kudu Jenkins: Verified

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If0c7ae9dfbd99c2c9a38e8b1f7748c70882e06b4
Gerrit-Change-Number: 9136
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] util: move ListFilesInDir to env util

[Andrew Wong (Code Review)]

Andrew Wong has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9166 )

Change subject: util: move ListFilesInDir to env_util
..

util: move ListFilesInDir to env_util

ListFilesInDir() is useful outside of just inspecting external mini
clusters. This patch moves it into env_util.

Change-Id: I8372c13b28c3c3058322fc96b656ef71aa18fc0a
Reviewed-on: http://gerrit.cloudera.org:8080/9166
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves 
---
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.h
M src/kudu/integration-tests/multidir_cluster-itest.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
5 files changed, 32 insertions(+), 26 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  David Ribeiro Alves: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8372c13b28c3c3058322fc96b656ef71aa18fc0a
Gerrit-Change-Number: 9166
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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

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

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

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

Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
..

KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client

The bulk of this commit is adding a new Thrift transport type,
SaslClientTransport, which facilitates SASL GSSAPI negotiation, as well
as integrity/privacy channel protection. The new transport is based on
Impala's version with some significant changes:

- Impala has a client and server SASL transport, necessitating a common
  superclass (SaslTransport). Since we only need a client transport, I
  collapsed all of the logic into a single class, which I think makes the
  code easier to follow.
- The transport uses Kudu helper types where possible, e.g., faststring
  buffers, and our existing SASL utility infrastructure.
- Integrity and privacy channel protection are implemented.

There are no standlone unit-tests for the transport, since that would
require implementing the server-specific counterpart. Instead, the class
is tested indirectly through using the HMS client to communicate with a
Kerberos-enabled HMS instance.

Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
---
M src/kudu/hms/CMakeLists.txt
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
A src/kudu/hms/sasl_client_transport.cc
A src/kudu/hms/sasl_client_transport.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/rpc/client_negotiation.cc
M src/kudu/rpc/sasl_common.cc
M src/kudu/rpc/sasl_common.h
M src/kudu/rpc/server_negotiation.cc
14 files changed, 959 insertions(+), 75 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8f217ae05fd36c8ee88fe20eeccd73d49233a345
Gerrit-Change-Number: 8692
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling

[Dan Burkert (Code Review)]

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

Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
..


Patch Set 8:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/hms_client-test.cc@219
PS8, Line 219: IsEndOfFile
> It's inconsistent with KRPC behavior though, which is confusing. I think it
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
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 
Gerrit-Comment-Date: Wed, 31 Jan 2018 20:08:22 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling

[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/8494

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

Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
..

KUDU-2191 (4/n): HMS Thrift client fault handling

This commit improves the new HMS Thrift client's ability to handle
faults. In particular:

- The client now uses send, receive, and connect timeouts so that a
  non-responsive HMS instance will not block the client indefinitely.
- The Thrift logging callback is hooked up to glog so that we get proper
  log messages from Thrift.
- The HmsClient class and method docs include information about behavior
  when errors are encountered.

In the part 2 review, Todd also brought up the prospect of creating a
wrapper Thrift socket or transport to inject slow log warning messages
automatically. I've held off doing this for now, because I haven't been
able to figure out a way to do that which can associate the slowness
with higher-level operations like 'create database', as opposed to
lower-level like 'socket write'. I've made sure to apply the slow
warning calls uniformly across the HmsClient methods, and I don't think
it will be too onerous to keep them consistent in the future.

Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
6 files changed, 191 insertions(+), 23 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 11
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 (5/n): Add Kerberos SASL support to the HMS client

[Dan Burkert (Code Review)]

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

Change subject: KUDU-2191 (5/n): Add Kerberos SASL support to the HMS client
..


Patch Set 5:

(24 comments)

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

http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.h@82
PS5, Line 82: "127.0.0.1"
> Are we sure that this won't clash with some other test services that may be
Yes, it uses an ephemeral port.  If it's shutdown and started again it will use 
the same port, but ephemeral ports tend not to get used again immediately.


http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:

http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.cc@68
PS5, Line 68:   CHECK(!krb5_conf.empty());
> worth CHECK(!hms_process_) here too to ensure that it's set up before start
Done


http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/mini_hms.cc@240
PS5, Line 240:
 :   
 : hadoop.rpc.protection
 : $5
 :   
> odd this is in hive-site and not core-site... but guess it works
Despite the property name this is applying to the HMS thrift server interface, 
not an HRPC interface.


http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc
File src/kudu/hms/sasl_client_transport.cc:

http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@53
PS5, Line 53: Width
> "width" strikes me as odd relative to "Size"
Done


http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@56
PS5, Line 56: kFrameHeaderWidth
> nit: Same as Todd's comment above.
Done


http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@107
PS5, Line 107: sasl_helper_.EnableGSSAPI();
> Does this mean this won't work with SASL plain?
Correct, the HMS doesn't appear to use SASL plain.


http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@120
PS5, Line 120:   transport_->open();
> DCHECK(!isOpen());
Done


http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@123
PS5, Line 123: ...
> catch(TException& e) ?
That doesn't catch all exceptions.


http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@157
PS5, Line 157: ResetReadBuf();
> are we guaranteed that read_slice_.data() != read_buf_.data()? ie it's alwa
Done


http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@177
PS5, Line 177:   // If we have some data in the read buffer, then return it.
 :   if (!read_slice_.empty()) {
 : return read_fast();
 :   }
 :
 :   // Otherwise, read a new frame from the wire and return it.
 :   ReadFrame();
 :   return read_fast();
> this code is a bit odd to me. why not just do:
Done


http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@224
PS5, Line 224:   ResetWriteBuf();
> the shrink_to_fit() in here seems a bit odd -- if you're doing a multi-fram
Yes, this is the only place I could find that was appropriate for releasing the 
write buffer.  Otherwise the connection will hold on to the buffer 
indefinitely, and never shrink it.


http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@254
PS5, Line 254: !s.IsIncomplete() && !s.ok())
> sasl_client_step() could return SASL_OK or SASL_CONTINUE, both of which are
WrapSaslCall will convert SASL_CONTINUE to a Status::Incomplete value, and 
SASL_OK to a Status::OK, both of which are handled here.


http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@274
PS5, Line 274: needs_wrap_ = rpc::NeedsWrap(sasl_conn_.get());
> Based on how you're initializing the transport, this looks like it should b
I'm not following, in this line needs_wrap_ is being retrieved from the SASL 
connection, which negotiates it with the remote server.  Ultimately, whether 
auth-conf, auth-int or no wrap is needed is determined by server configuration.


http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@284
PS5, Line 284: payload.size()
> nit: Not that it's likely, but Slice objects can have lengths greater than
Done


http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@292
PS5, Line 292:   // Read the fixed-length message header.
> DCHECK_EQ(payload.size(), 0);
I've changed it so that payload gets overwritten, so the caller doesn't need to 
clear.


http://gerrit.cloudera.org:8080/#/c/8692/5/src/kudu/hms/sasl_client_transport.cc@343
PS5, Line 343:   s = rpc::AllowProtection(sasl_conn_.get());
 :   if (!s.ok()) {
 : throw SaslException(s);
 :   }
> Integrity and confidentiality is set as a strict requirement here. 

[kudu-CR] KUDU-1613: evict replicas with wrong server UUIDs

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9136 )

Change subject: KUDU-1613: evict replicas with wrong server UUIDs
..


Patch Set 6: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0c7ae9dfbd99c2c9a38e8b1f7748c70882e06b4
Gerrit-Change-Number: 9136
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 31 Jan 2018 19:45:12 +
Gerrit-HasComments: No

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8830 )

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..


Patch Set 14:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/common/partial_row-test.cc
File src/kudu/common/partial_row-test.cc:

http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/common/partial_row-test.cc@227
PS20, Line 227:   // the copy constructor.
> This error message may be clearer if it printed the scaled value instead of
Done


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

http://gerrit.cloudera.org:8080/#/c/8830/14/src/kudu/common/types.h@564
PS14, Line 564: return _UNSCALED_DECIMAL64;
> Take a look at column_predicate-test for examples of type specific boundary
I added a few tests in column_predicate-test but also simplified this code not 
to include the decimal "max" values but instead stick to the physical type 
maximums.

In a follow up patch I will update places we use IsMinValue() and IsMaxValue() 
to take the ColumnTypeAtrributes into consideration and calculate the "real" 
limits for decimal columns. Until then the predicates are less than optimal but 
still correct.


http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.h
File src/kudu/util/decimal_util.h:

http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.h@63
PS20, Line 63:
> Is having a default scale a benefit here?  I'd expect that if the scale is
True. I will remove the default.


http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc
File src/kudu/util/decimal_util.cc:

http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc@28
PS20, Line 28:   // 38 digits, 1 extra leading zero, decimal point,
> Needs test
Done


http://gerrit.cloudera.org:8080/#/c/8830/20/src/kudu/util/decimal_util.cc@32
PS20, Line 32:   int128_t n = d < 0? -d : d;
> I think a for loop would be more idiomatic here.
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 14
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 31 Jan 2018 19:42:36 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

[Grant Henke (Code Review)]

Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, Adar Dembo, Todd 
Lipcon,

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

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

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

Change subject: KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)
..

KUDU-721: Support for Decimal type, Part 1 (Server, C++ Client)

Introduces the Decimal type to the server and
C++ client. Follow on work will enhance the
utility in the client and add support to the
Java client and other integrations and add
documentation.

The decimal type has column type attributes to
support the “parameterized type”. The precision
and scale column type attributes are not stored
with each value, but instead are leveraged to map
to a correctly sized internal type
(DECIMAL32, DECIMAL64, DECIMAL128). These
internal types are represented and stored as
equivalently sized integers.

This also removes the int128 suffixes because
they break the client by using C++11 features.
They may be added back in a different way later.

Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
---
M src/kudu/client/CMakeLists.txt
M src/kudu/client/predicate-test.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/common.proto
M src/kudu/common/partial_row-test.cc
M src/kudu/common/partial_row.cc
M src/kudu/common/partial_row.h
M src/kudu/common/schema-test.cc
M src/kudu/common/schema.cc
M src/kudu/common/schema.h
M src/kudu/common/types.cc
M src/kudu/common/types.h
M src/kudu/common/wire_protocol.cc
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/all_types-itest.cc
A src/kudu/integration-tests/decimal-itest.cc
M src/kudu/util/CMakeLists.txt
A src/kudu/util/decimal_util-test.cc
A src/kudu/util/decimal_util.cc
A src/kudu/util/decimal_util.h
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
32 files changed, 1,337 insertions(+), 169 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b06142f43c66973f36376bd2c88ca6f8d9f7632
Gerrit-Change-Number: 8830
Gerrit-PatchSet: 21
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1613: evict replicas with wrong server UUIDs

[Andrew Wong (Code Review)]

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

Change subject: KUDU-1613: evict replicas with wrong server UUIDs
..


Patch Set 6:

Whoops, the previous rev still had a typo.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0c7ae9dfbd99c2c9a38e8b1f7748c70882e06b4
Gerrit-Change-Number: 9136
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 31 Jan 2018 19:07:58 +
Gerrit-HasComments: No

[kudu-CR] KUDU-1613: evict replicas with wrong server UUIDs

[Andrew Wong (Code Review)]

Hello Tidy Bot, Alexey Serbin, David Ribeiro Alves, Kudu Jenkins,

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

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

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

Change subject: KUDU-1613: evict replicas with wrong server UUIDs
..

KUDU-1613: evict replicas with wrong server UUIDs

Currently, if a tablet server is wiped and restarted at the same host
and port with a new UUID assigned to it before the replicas on its
previous incarnation are evicted, those replicas won't ever be evicted.
With this patch, upon receiving a response with WRONG_SERVER_UUID,
leaders will treat the follower as failed and handle it appropriately,
re-replicating them elsewhere.

A new test case is added to raft_consensus-itest that is set up to
prompt eviction (i.e. low timeouts, ample servers to replicate to), and
without the change, this test fails.

Change-Id: If0c7ae9dfbd99c2c9a38e8b1f7748c70882e06b4
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.h
3 files changed, 95 insertions(+), 7 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0c7ae9dfbd99c2c9a38e8b1f7748c70882e06b4
Gerrit-Change-Number: 9136
Gerrit-PatchSet: 6
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-1613: evict replicas with wrong server UUIDs

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9136 )

Change subject: KUDU-1613: evict replicas with wrong server UUIDs
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9136/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9136/4//COMMIT_MSG@9
PS4, Line 9: rebuilt
> Done, although this is documented here: http://kudu.apache.org/docs/adminis
right, but that mentions "rebuilding the file system layout". my comment was 
not that "rebuilt' was the wrong term, just that a reader might not get what it 
means without the remaining part.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0c7ae9dfbd99c2c9a38e8b1f7748c70882e06b4
Gerrit-Change-Number: 9136
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 31 Jan 2018 18:52:05 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1613: evict replicas with wrong server UUIDs

[Andrew Wong (Code Review)]

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

Change subject: KUDU-1613: evict replicas with wrong server UUIDs
..


Patch Set 4:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/9136/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9136/4//COMMIT_MSG@9
PS4, Line 9: rebuilt
> what does "rebuilt" mean?
Done, although this is documented here: 
http://kudu.apache.org/docs/administration.html#rebuilding_kudu


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/consensus/consensus_peers.cc
File src/kudu/consensus/consensus_peers.cc:

http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/consensus/consensus_peers.cc@320
PS4, Line 320: case TabletServerErrorPB::TABLET_FAILED:
> add fallthrough expected or something
Done


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/consensus/consensus_peers.cc@321
PS4, Line 321: case TabletServerErrorPB::WRONG_SERVER_UUID:
> nit: might make sense to reverse the order of the cases (have TabletServerE
Done


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2750
PS4, Line 2750: test that ensures when we start a new tablet server using the
  : // same ports, existing tablets that previously used those 
ports will be
  : // evicted and replicated elsewhere.
> "tablets that used those ports"? this is confusing, please rephrase. maybe:
Done


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2755
PS4, Line 2755: is_3_4_3
> this is tied to a layout (3 replicas) could you just use the flag name inst
Done


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2805
PS4, Line 2805: env_->CreateDir
> check status
Done


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2806
PS4, Line 2806: env_->CreateDir
> check status
Done


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2809
PS4, Line 2809: new_ts->Start();
> check status, here and elsewhere
Done


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2810
PS4, Line 2810: LOG(INFO) << Substitute("Created new tablet server: $0 ($1)",
  :   new_ts->uuid(), ts_opts.rpc_bind_address.ToString());
> did you mean to leave this. doesn't seem super informative...
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0c7ae9dfbd99c2c9a38e8b1f7748c70882e06b4
Gerrit-Change-Number: 9136
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 31 Jan 2018 18:36:21 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1613: evict replicas with wrong server UUIDs

[Andrew Wong (Code Review)]

Hello Tidy Bot, Alexey Serbin, David Ribeiro Alves, Kudu Jenkins,

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

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

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

Change subject: KUDU-1613: evict replicas with wrong server UUIDs
..

KUDU-1613: evict replicas with wrong server UUIDs

Currently, if a tablet server is wiped and restarted at the same host
and port with a new UUID to it before the replicas on it are evicted,
they won't ever be evicted. With this patch, upon receiving a response
with WRONG_SERVER_UUID, leaders will treat the follower as failed and
handle it appropriately.

A new test case is added to raft_consensus-itest that is set up to
prompt eviction (i.e. low timeouts, ample servers to replicate to), and
without the change, it fails.

Change-Id: If0c7ae9dfbd99c2c9a38e8b1f7748c70882e06b4
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/mini-cluster/external_mini_cluster.h
3 files changed, 95 insertions(+), 7 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If0c7ae9dfbd99c2c9a38e8b1f7748c70882e06b4
Gerrit-Change-Number: 9136
Gerrit-PatchSet: 5
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] util: move ListFilesInDir to env util

[Andrew Wong (Code Review)]

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


Change subject: util: move ListFilesInDir to env_util
..

util: move ListFilesInDir to env_util

ListFilesInDir() is useful outside of just inspecting external mini
clusters. This patch moves it into env_util.

Change-Id: I8372c13b28c3c3058322fc96b656ef71aa18fc0a
---
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.cc
M src/kudu/integration-tests/external_mini_cluster_fs_inspector.h
M src/kudu/integration-tests/multidir_cluster-itest.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
5 files changed, 32 insertions(+), 26 deletions(-)



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

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

[kudu-CR] Don't rely on pending config OpId index for peer promotion

[Todd Lipcon (Code Review)]

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

Change subject: Don't rely on pending config OpId index for peer promotion
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9161/3/src/kudu/consensus/consensus_queue.cc
File src/kudu/consensus/consensus_queue.cc:

http://gerrit.cloudera.org:8080/#/c/9161/3/src/kudu/consensus/consensus_queue.cc@a982
PS3, Line 982:
> true, but what I guess I was saying is. do we care?
That's fair. It's possible would have lost leadership by the time the 
notification executes, though, right? I guess we only woudl end up with a WARN 
in that case.

Curious for Mike's take on why the original "binding" of the notification to 
its original term was considered important.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a033ce16f626458efdb59b1b4dd8450e8591761
Gerrit-Change-Number: 9161
Gerrit-PatchSet: 3
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 31 Jan 2018 18:34:04 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1613: evict replicas with wrong server UUIDs

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9136 )

Change subject: KUDU-1613: evict replicas with wrong server UUIDs
..


Patch Set 4:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/9136/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/9136/4//COMMIT_MSG@9
PS4, Line 9: rebuilt
what does "rebuilt" mean?


http://gerrit.cloudera.org:8080/#/c/9136/4//COMMIT_MSG@15
PS4, Line 15: ample servers to replicated to
grammar


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/consensus/consensus_peers.cc
File src/kudu/consensus/consensus_peers.cc:

http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/consensus/consensus_peers.cc@320
PS4, Line 320: case TabletServerErrorPB::TABLET_FAILED:
add fallthrough expected or something


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/consensus/consensus_peers.cc@321
PS4, Line 321: case TabletServerErrorPB::WRONG_SERVER_UUID:
nit: might make sense to reverse the order of the cases (have 
TabletServerErrorPB::TABLET_FAILED last) and add a comment indicating we treat 
WRONG_SERVER_UUID as tablet failed


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/external_mini_cluster_fs_inspector.h
File src/kudu/integration-tests/external_mini_cluster_fs_inspector.h:

http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/external_mini_cluster_fs_inspector.h@a52
PS4, Line 52:
pull this to another changerequest?


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:

http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2750
PS4, Line 2750: test that ensures when we start a new tablet server using the
  : // same ports, existing tablets that previously used those 
ports will be
  : // evicted and replicated elsewhere.
"tablets that used those ports"? this is confusing, please rephrase. maybe:
"Test that when we reset and restart a tablet server, with a new uuid but with 
the same host and ports, replicas that were hosted by the previous incarnation 
are correctly detected as failed and eventually re-replicated."


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2755
PS4, Line 2755: is_3_4_3
this is tied to a layout (3 replicas) could you just use the flag name instead? 
(i.e. "prepare_replacement_before_eviction")


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2802
PS4, Line 2802: bount
typo


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2805
PS4, Line 2805: env_->CreateDir
check status


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2806
PS4, Line 2806: env_->CreateDir
check status


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2809
PS4, Line 2809: new_ts->Start();
check status, here and elsewhere


http://gerrit.cloudera.org:8080/#/c/9136/4/src/kudu/integration-tests/raft_consensus-itest.cc@2810
PS4, Line 2810: LOG(INFO) << Substitute("Created new tablet server: $0 ($1)",
  :   new_ts->uuid(), ts_opts.rpc_bind_address.ToString());
did you mean to leave this. doesn't seem super informative...



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If0c7ae9dfbd99c2c9a38e8b1f7748c70882e06b4
Gerrit-Change-Number: 9136
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Wed, 31 Jan 2018 17:23:35 +
Gerrit-HasComments: Yes