[kudu-CR] KUDU-2254: Detect and warn about misusage of KuduContext

[Hao Hao (Code Review)]

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


Change subject: KUDU-2254: Detect and warn about misusage of KuduContext
..

KUDU-2254: Detect and warn about misusage of KuduContext

Since KuduContext carries a bunch of information for client connection,
such as authentication token, KuduContext should be created in the
driver and shared with executors. It would be great to provide a way
to warn users (e.g Logging) about such behavior since this kind of
issues are very subtle to detect.

This patch adds logic to detect and warn about this kind of misusage of
KuduContecx based on TaskContext information. A corresponding unit test
is also added.

Change-Id: I65a7cd11a14fa8a668079b0d1fcf6ed3a34fb652
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M 
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala
2 files changed, 43 insertions(+), 1 deletion(-)



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

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

[kudu-CR] [tls socket-test] fix test to pass on Ubuntu 16.04

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,

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

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

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

Change subject: [tls_socket-test] fix test to pass on Ubuntu 16.04
..

[tls_socket-test] fix test to pass on Ubuntu 16.04

Prior to this fix, the TlsSocketTest.TestNonBlockingWritev test failed
on Ubuntu 16.04 (kernel 4.4.0).  After some investigation, it turned
out that setting socket send buffer size for something lower than the
MTU size (which is 64kB for localhost) triggered the delayed
acknowledgement logic.  Since the test passes big chunks of data
(32MB on every iteration), with 40ms delay for almost every 16KB sent,
the test eventually timed out.  I verified that disabling the delayed
TCP ack for TLS sockets fixed the issue.

I think that the proper fix is to remove the custom setting for the
socket send buffer: a 32MB chunk of data is big enough to not fit into
the socket buffer of the default size.

Also, it seems this issue is not going to bite us in real life since
we don't set the size of the socket send buffer anywhere.

NOTE: however, some mystery is left since the failing test would pass
  if running under strace.

Change-Id: I94170d5bdbe17a5952a82faf74d8b868b42460aa
---
M src/kudu/security/tls_socket-test.cc
1 file changed, 0 insertions(+), 4 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94170d5bdbe17a5952a82faf74d8b868b42460aa
Gerrit-Change-Number: 8996
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2202: support for removing data directories (take two)

[Adar Dembo (Code Review)]

Hello Kudu Jenkins, Andrew Wong, Todd Lipcon,

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

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

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

Change subject: KUDU-2202: support for removing data directories (take two)
..

KUDU-2202: support for removing data directories (take two)

This commit extends "kudu fs update_dirs" to allow the removal of data
directories. A data dir may be removed provided there are no tablets
configured to use it. The --force flag may be used to override this safety
check; tablets that depend on the removed data dir will fail to bootstrap
the next time the server is started.

I'll be the first to admit that this functionality isn't terribly useful at
the moment because Kudu deployments are configured to stripe data across all
data directories, so removing a data dir will fail all tablets, which is
equivalent to reformatting the node. Nevertheless, it will become more
useful as users customize the new --fs_target_data_dirs_per_tablet gflag.

This is the second attempt at implementing this functionality. The first
used a different interface and was vulnerable to a data loss issue
documented in KUDU-2202. Andrew fixed that in commit 47b81c4, which reopens
the door for safely removing data directories.

I considered a stronger check that only prohibits data dir removal if there
were data blocks on the to-be-removed data dir, but decided against it:
- It's rare to find a tablet configured to use a data dir but without any
  actual blocks on it. In fact, that's only likely for empty tables, or
  tables with empty range partitions.
- We'd still need to rewrite any unaffected tablet superblocks and remove
  the data dir from their data dir groups.

There are several interesting modifications here:
1. PathInstanceMetadataFile::CheckIntegrity can now be run without checking
   for missing instance files. This is used when removing a data dir (its
   instance file will be missing).
2. The DataDirManager and FsManager may now be constructed with both
   read_only=true and update_on_disk=true. The CLI tool leverages this
   functionality to create a "speculative" FsManager using the new data dir
   configuration that is used to check whether existing tablets depend on
   the data dir to be removed.
3. The DataDirManager now checks for integrity after making on-disk changes.
   While not strictly necessary, this seems like a good sanity check to
   avoid introducing bugs.
4. The "kudu fs update_dirs" action was updated for data dir removal. It now
   accepts the --force option, without which removal will only be allowed if
   no tablets are configured to use the removed data dir.

Change-Id: Iacb650115bcdf055542ef2777641a3201fc8e30b
---
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/block_manager_util.h
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
10 files changed, 343 insertions(+), 90 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iacb650115bcdf055542ef2777641a3201fc8e30b
Gerrit-Change-Number: 8978
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2202: support for removing data directories (take two)

[Adar Dembo (Code Review)]

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

Change subject: KUDU-2202: support for removing data directories (take two)
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8978/3/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:

http://gerrit.cloudera.org:8080/#/c/8978/3/src/kudu/fs/data_dirs.h@222
PS3, Line 222: without updating the latter
> nit: the naming here gives me pause since the name is no longer accurate, b
Yeah, I couldn't think of a better name either.


http://gerrit.cloudera.org:8080/#/c/8978/3/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:

http://gerrit.cloudera.org:8080/#/c/8978/3/src/kudu/fs/data_dirs.cc@810
PS3, Line 810:  the missing directory made its way
 : // into the list of data directories.
> nit: at first glance, I read this as, "We have a missing directory and it i
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacb650115bcdf055542ef2777641a3201fc8e30b
Gerrit-Change-Number: 8978
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 11 Jan 2018 03:47:40 +
Gerrit-HasComments: Yes

[kudu-CR] [tls socket-test] fix test to pass at Ubuntu 16.04

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,

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

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

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

Change subject: [tls_socket-test] fix test to pass at Ubuntu 16.04
..

[tls_socket-test] fix test to pass at Ubuntu 16.04

Prior to this fix, the TlsSocketTest.TestNonBlockingWritev test failed
on Ubuntu 16.04 (kernel 4.4.0).  After some investigation, it turned
out that setting socket send buffer size for something lower than the
MTU size (which is 64kB for localhost) triggers the delayed
acknowledgement logic to send ack for the received data after 40ms
timeout.  Since the test passes big chunks of data
(32MB on every iteration), with 40ms delay for almost every 16KB sent,
the test eventually timed out.  I verified that disabling the delayed
TCP ack for TLS sockets fixed the issue.

I think that the proper fix is to remove the custom setting for the
socket send buffer: a 32MB chunk of data is big enough to not fit into
the socket buffer of the default size.

Also, it seems this issue is not going to bite us in real life since
we don't set the size of the socket send buffer anywhere.

NOTE: however, some mystery is left since the failing test would pass
  if running under strace.

Change-Id: I94170d5bdbe17a5952a82faf74d8b868b42460aa
---
M src/kudu/security/tls_socket-test.cc
1 file changed, 0 insertions(+), 4 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94170d5bdbe17a5952a82faf74d8b868b42460aa
Gerrit-Change-Number: 8996
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [tls socket-test] fix test to pass at Ubuntu 16.04

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,

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

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

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

Change subject: [tls_socket-test] fix test to pass at Ubuntu 16.04
..

[tls_socket-test] fix test to pass at Ubuntu 16.04

Prior to this fix, the TlsSocketTest.TestNonBlockingWritev test failed
on Ubuntu 16.04 (kernel 4.4.0).  After some investigation, it turned
out that setting socket send buffer size for something lower than MTU
triggers the delayed acknowledgement logic to send ack for received
data after 40ms timeout.  Since the test passes big chunks of data
(32MB on every iteration), with 40ms delay for almost every 16KB sent,
the test eventually timed out.  I verified that disabling the delayed
TCP ack for TLS sockets fixed the issue.

I think that the proper fix is to remove the custom setting for the
socket send buffer: a 32MB chunk of data is big enough to not fit into
the socket buffer of the default size.

Also, it seems this issue is not going to bite us in real life since
we don't set the size of the socket send buffer anywhere.

NOTE: however, some mystery is left since the failing test would pass
  if running under strace.

Change-Id: I94170d5bdbe17a5952a82faf74d8b868b42460aa
---
M src/kudu/security/tls_socket-test.cc
1 file changed, 0 insertions(+), 4 deletions(-)


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

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

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

[Todd Lipcon (Code Review)]

Todd Lipcon 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 8:

(23 comments)

I think this could do with a couple end-to-end tests from the client writing 
decimals of different precision and making sure they come back reasonably

http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG@17
PS8, Line 17: stored
not stored? it must be stored in the metadata somewhere right?


http://gerrit.cloudera.org:8080/#/c/8830/8//COMMIT_MSG@24
PS8, Line 24: int128 suffixes
perhaps we should guard them with a preprocessor check for c++11 instead?


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h
File src/kudu/client/schema.h:

http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@63
PS8, Line 63: class KuduColumnTypeAttributes {
should this be exported for users? If so I think we need to PIMPL it for ABI 
compatibility (I imagine this is where we might later add stuff like charsets 
for strings?)

If not exported then I think it belongs in schema-internal.h or somesuch


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@177
PS8, Line 177:   /// @todo KUDU-809: make this hard-to-use constructor private.
 :   ///   Clients should use the Builder API. Currently only the 
Python API
 :   ///   uses this old API.
this has been deprecated for several releases now and I dont think python uses 
it anymore. Maybe we can remove it (make it private)?  Changing its signature 
is equally as illegal as removing it.


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@323
PS8, Line 323: floating-point
it's not floating point, right?


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@328
PS8, Line 328:   /// The precision must be between 0 and 38.
curious what a precision of 0 means... that can only store the value 0?


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.h@343
PS8, Line 343:   /// columns precision.
typo: column's


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc
File src/kudu/client/schema.cc:

http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@133
PS8, Line 133:   return kudu::DECIMAL128;
nit: weird indentation


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@153
PS8, Line 153: case kudu::DECIMAL32 : return KuduColumnSchema::DECIMAL;
nit: extra space


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@266
PS8, Line 266:   return Status::InvalidArgument("no scale provided for 
decimal column", data_->name);
is a default scale of 0 not reasonable? I seem to recall that sql allows 
DECIMAL(5) and that means 0 through 9


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@285
PS8, Line 285:   }
can we check that no precision/scale are specified for non-decimal column 
types? want to make sure someone doesn't try to do STRING(10) under some 
assumption this would produce a fixed-length or truncated string type (as in 
sql VARCHAR(10))


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@287
PS8, Line 287:   int32_t precision = 0;
 :   if (data_->has_precision) {
 : precision = data_->precision;
 :   }
think ternary would be easier to read here and below


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@297
PS8, Line 297:   KuduColumnTypeAttributes kuduColumnTypeAttributes =
nit: var name style
Also you can just construct this like KuduColumnTypeAttributes attr(precision, 
scale) -- no need for the extra assignment


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/client/schema.cc@641
PS8, Line 641: typeAttrs
nit: style


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

http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@49
PS8, Line 49:99) * 100) + 99; // 38 9's
well this is fun. have you tested that this works as expected and not getting 
secretly truncated somewhere along the line?


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@53
PS8, Line 53:static const int32_t MIN_DECIMAL_PRECISION = 0;
> warning: invalid case style for global constant 'MIN_DECIMAL_PRECISION' [re
see comment elsewhere about precision 0


http://gerrit.cloudera.org:8080/#/c/8830/8/src/kudu/common/decimal.h@63
PS8, Line 63:explicit Decimal(int128_t s) :
I sort of feel like this Decimal instance should retain its precision and scale 
as a member and then check for compatibility in the appropriate spots. 
Otherwise it will be very easy to get incorrect results if you accidentally 
pass a non-matching decimal Value into something like a 

[kudu-CR] [webui] Make tombstone tablet info less confusing

[Will Berkeley (Code Review)]

Will Berkeley has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8981 )

Change subject: [webui] Make tombstone tablet info less confusing
..

[webui] Make tombstone tablet info less confusing

Previously, when a tombstone tablet was reloaded at server startup,
the last status message was "Tablet initializing...". This was
confusing as it set the expectation that something more was going
to happen to the tombstoned tablet. The message is now simply
"Tombstoned".

Also, now that tombstoned tablets can vote, they retain cmeta
despite not participating in non-election Raft operations.
Their list of peers is not updated and not usually relevant. It
might be confusing to see it on the web ui. This patch suppresses
it.

Change-Id: I5c879cc7ff634e5b434fc33374d3010cf1f262cb
Reviewed-on: http://gerrit.cloudera.org:8080/8981
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/tablet/tablet_replica.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/tserver_path_handlers.cc
4 files changed, 56 insertions(+), 6 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5c879cc7ff634e5b434fc33374d3010cf1f262cb
Gerrit-Change-Number: 8981
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] fs: update default data dir group size

[Todd Lipcon (Code Review)]

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

Change subject: fs: update default data dir group size
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8995/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8995/3//COMMIT_MSG@10
PS3, Line 10: sets the new default to 3. Upon experimenting with the flag via 
YCSB
I think we should also try an analytic workload like tpch_real_world to load 
lineitem at a scale factor larger than memory (eg 1TB per node) and then scan 
it with Impala to make sure that it's not significantly slower.


http://gerrit.cloudera.org:8080/#/c/8995/3/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:

http://gerrit.cloudera.org:8080/#/c/8995/3/src/kudu/fs/data_dirs.cc@69
PS3, Line 69:  "data directories.");
we should also document the effect this setting has on fault tolerance



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2dd0d3f8cf140f684318c0dffb45a1091302ecdc
Gerrit-Change-Number: 8995
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 11 Jan 2018 02:17:50 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2236: use debug logging where appropriate

[Todd Lipcon (Code Review)]

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

Change subject: KUDU-2236: use debug logging where appropriate
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8961/2/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/8961/2/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java@a347
PS2, Line 347:
does this already get logged at debug level elsewhere?


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

http://gerrit.cloudera.org:8080/#/c/8961/2/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java@688
PS2, Line 688:   LOG.warn(msg);
does this need to be warned even in the non-explicit disconnect case? would it 
already have been logged elsewhere? seems like this is a net new log added by 
this patch?


http://gerrit.cloudera.org:8080/#/c/8961/2/java/kudu-client/src/test/java/org/apache/kudu/util/CapturingLogAppender.java
File 
java/kudu-client/src/test/java/org/apache/kudu/util/CapturingLogAppender.java:

http://gerrit.cloudera.org:8080/#/c/8961/2/java/kudu-client/src/test/java/org/apache/kudu/util/CapturingLogAppender.java@60
PS2, Line 60:* Sets the Log4j log level to read log messages from.
is this inclusive? perhaps rename to 'setMinimumLevel' or somesuch to indicate 
that it's this level and above?


http://gerrit.cloudera.org:8080/#/c/8961/2/java/kudu-client/src/test/java/org/apache/kudu/util/CapturingLogAppender.java@64
PS2, Line 64: logger.setLevel(level);
does this have the side effect of actually disabling debug logs while the 
capturing appender is active? Also it doesn't seem to restore it afterwards?

Perhaps instead of changing the underlying logger level we should just be 
filtering in the append() call?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I780df2eeb51a14b65dd4283dfe9d4d6daf909661
Gerrit-Change-Number: 8961
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 11 Jan 2018 02:11:50 +
Gerrit-HasComments: Yes

[kudu-CR] [docs] Added steps to update HMS after migrating to multiple Kudu masters

[Todd Lipcon (Code Review)]

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

Change subject: [docs] Added steps to update HMS after migrating to multiple 
Kudu masters
..


Patch Set 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8948/8/docs/administration.adoc
File docs/administration.adoc:

http://gerrit.cloudera.org:8080/#/c/8948/8/docs/administration.adoc@263
PS8, Line 263: statement
maybe it's just me, but seems like this isn't super clear since this statement 
here needs to be done in the Impala shell, whereas the example below needs to 
be done against the undrelying HMS backend database.

Perhaps we can explicitly say that this statement should be executed in Impala.


http://gerrit.cloudera.org:8080/#/c/8948/8/docs/administration.adoc@380
PS8, Line 380: update the HMS database manually.
 : * The following is an example SQL statement:
per above, I think we should be explicit that this statement must be run 
against the underlying SQL database that provides storage for the HMS. ie *not* 
via the Hive or Impala shells.


http://gerrit.cloudera.org:8080/#/c/8948/8/docs/security.adoc
File docs/security.adoc:

http://gerrit.cloudera.org:8080/#/c/8948/8/docs/security.adoc@102
PS8, Line 102: === Client Authentication to Secure Kudu Clusters
it seems this change snuck into the wrong git commit?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iab3999c9e581ed3591b220c08491cdae867c91db
Gerrit-Change-Number: 8948
Gerrit-PatchSet: 8
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 11 Jan 2018 02:07:09 +
Gerrit-HasComments: Yes

[kudu-CR] fs: update default data dir group size

[Andrew Wong (Code Review)]

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

Change subject: fs: update default data dir group size
..


Patch Set 3:

> Patch Set 2:
>
> Interesting that this change is causing build failures of the randomized disk 
> failure test. I think the lower group size is yielding fewer fsyncs for the 
> tablet, since it's just not tripping the fault injection.

Much simpler, the smaller disk group meant that failing the first non-metadata 
data directory didn't always work.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2dd0d3f8cf140f684318c0dffb45a1091302ecdc
Gerrit-Change-Number: 8995
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 11 Jan 2018 02:02:04 +
Gerrit-HasComments: No

[kudu-CR] [docs] Document how to recover from a majority failed tablet

[Todd Lipcon (Code Review)]

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

Change subject: [docs] Document how to recover from a majority failed tablet
..


Patch Set 5:

(4 comments)

This doc is nice. I wonder if we could automate the whole thing, though, into 
something like 'kudu tablet unsafe_promote_minority' or somesuch? (not that we 
shouldn't commit this in the meantime)

http://gerrit.cloudera.org:8080/#/c/8402/5/docs/administration.adoc
File docs/administration.adoc:

http://gerrit.cloudera.org:8080/#/c/8402/5/docs/administration.adoc@809
PS5, Line 809: that's
style: I think it's easier to read "that has"


http://gerrit.cloudera.org:8080/#/c/8402/5/docs/administration.adoc@812
PS5, Line 812:  Permanent data loss is
 : possible
I think this isn't quite clear that permanent data loss is possible _by 
following these steps_. ie even if you run these steps, you may have lost the 
most recent edits from the tablet. The way it's written makes it sound "maybe 
these steps wont work"


http://gerrit.cloudera.org:8080/#/c/8402/5/docs/administration.adoc@839
PS5, Line 839: To revive the tablet
maybe here say something like "to accept the potential data loss and restore 
from the remaining replica"


http://gerrit.cloudera.org:8080/#/c/8402/5/docs/administration.adoc@845
PS5, Line 845: r tserver-00
nit: use `...` around hostnames



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic6326f65d029a1cd75e487b16ce5be4baea2f215
Gerrit-Change-Number: 8402
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Thu, 11 Jan 2018 02:01:45 +
Gerrit-HasComments: Yes

[kudu-CR] fs: update default data dir group size

[Andrew Wong (Code Review)]

Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,

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

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

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

Change subject: fs: update default data dir group size
..

fs: update default data dir group size

This patch makes fs_target_data_dirs_per_tablet non-experimental and
sets the new default to 3. Upon experimenting with the flag via YCSB
workloads, only read workload tail latency seemed to be affected at very
small group sizes (e.g. 1). 3 seems like a reasonable choice, and we can
always update it in the future.

This patch updates TabletServerDiskFailureTest::TestRandomOpSequence to
fail all data directories but the metadata directory, rather than just
the first non-metadata data dir. The previous failure injection
configuration wouldn't always select a data directory with blocks in it.

Note that existing data will not be updated with the new flag. Only new
tablets will honor the sizing.

Change-Id: I2dd0d3f8cf140f684318c0dffb45a1091302ecdc
---
M src/kudu/fs/data_dirs.cc
M src/kudu/tserver/tablet_server-test.cc
2 files changed, 12 insertions(+), 10 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2dd0d3f8cf140f684318c0dffb45a1091302ecdc
Gerrit-Change-Number: 8995
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [webui] Make tombstone tablet info less confusing

[Todd Lipcon (Code Review)]

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

Change subject: [webui] Make tombstone tablet info less confusing
..


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c879cc7ff634e5b434fc33374d3010cf1f262cb
Gerrit-Change-Number: 8981
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Thu, 11 Jan 2018 01:40:26 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2202: support for removing data directories (take two)

[Andrew Wong (Code Review)]

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

Change subject: KUDU-2202: support for removing data directories (take two)
..


Patch Set 3:

(5 comments)

Just a couple more nits from me

http://gerrit.cloudera.org:8080/#/c/8978/3/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:

http://gerrit.cloudera.org:8080/#/c/8978/3/src/kudu/fs/data_dirs.h@222
PS3, Line 222: without updating the latter
nit: the naming here gives me pause since the name is no longer accurate, but I 
can't settle on a more fitting name. Maybe update_instances? 
update_directories? Though maybe this comment is clear enough to not warrant 
any changes.


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

http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/data_dirs.cc@805
PS1, Line 805: // happen when opts_.read_only and opts_.update_on_disk are true 
(i.e.
 : // if this DataDirManager is trialing a new data dir 
confi
> OK, I took another stab at this comment. Let me know what you think.
Much clearer, thanks!


http://gerrit.cloudera.org:8080/#/c/8978/3/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:

http://gerrit.cloudera.org:8080/#/c/8978/3/src/kudu/fs/data_dirs.cc@810
PS3, Line 810:  the missing directory made its way
 : // into the list of data directories.
nit: at first glance, I read this as, "We have a missing directory and it is 
added to the list of data directories". Maybe flip it for clarity: "...the list 
of [available uuids?] accounts for the missing directory".


http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/tools/kudu-tool-test.cc@2232
PS1, Line 2232: // Make sure the failure reall
> We could, but is it necessary? We wouldn't expect tablets to just disappear
Ack


http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/tools/tool_action_fs.cc
File src/kudu/tools/tool_action_fs.cc:

http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/tools/tool_action_fs.cc@854
PS1, Line 854: "Updates the set of data directories in an existing Kudu 
filesystem")
 :   .ExtraDescription("If a data directory is in use by a 
tablet and is "
 :   "removed, the operation will fail unless --force is 
also used")
 :   .AddOptionalParameter("fs_wal_dir")
 :   .AddOptionalParameter("fs_data_dirs")
 :   .AddOptionalParameter("force", boost::none, string("If 
true, permits "
 :   "the removal of a data directory that is configured 
for use by "
 :   "existing tablets. Those tablets will fail the next 
time the server "
 :   "is started"))
> I'm not sure; as you can see, List uses 'tablets' instead of 'local replica
Ack



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacb650115bcdf055542ef2777641a3201fc8e30b
Gerrit-Change-Number: 8978
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 11 Jan 2018 00:58:45 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2202: support for removing data directories (take two)

[Adar Dembo (Code Review)]

Hello Andrew Wong, Todd Lipcon,

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

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

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

Change subject: KUDU-2202: support for removing data directories (take two)
..

KUDU-2202: support for removing data directories (take two)

This commit extends "kudu fs update_dirs" to allow the removal of data
directories. A data dir may be removed provided there are no tablets
configured to use it. The --force flag may be used to override this safety
check; tablets that depend on the removed data dir will fail to bootstrap
the next time the server is started.

I'll be the first to admit that this functionality isn't terribly useful at
the moment because Kudu deployments are configured to stripe data across all
data directories, so removing a data dir will fail all tablets, which is
equivalent to reformatting the node. Nevertheless, it will become more
useful as users customize the new --fs_target_data_dirs_per_tablet gflag.

This is the second attempt at implementing this functionality. The first
used a different interface and was vulnerable to a data loss issue
documented in KUDU-2202. Andrew fixed that in commit 47b81c4, which reopens
the door for safely removing data directories.

I considered a stronger check that only prohibits data dir removal if there
were data blocks on the to-be-removed data dir, but decided against it:
- It's rare to find a tablet configured to use a data dir but without any
  actual blocks on it. In fact, that's only likely for empty tables, or
  tables with empty range partitions.
- We'd still need to rewrite any unaffected tablet superblocks and remove
  the data dir from their data dir groups.

There are several interesting modifications here:
1. PathInstanceMetadataFile::CheckIntegrity can now be run without checking
   for missing instance files. This is used when removing a data dir (its
   instance file will be missing).
2. The DataDirManager and FsManager may now be constructed with both
   read_only=true and update_on_disk=true. The CLI tool leverages this
   functionality to create a "speculative" FsManager using the new data dir
   configuration that is used to check whether existing tablets depend on
   the data dir to be removed.
3. The DataDirManager now checks for integrity after making on-disk changes.
   While not strictly necessary, this seems like a good sanity check to
   avoid introducing bugs.
4. The "kudu fs update_dirs" action was updated for data dir removal. It now
   accepts the --force option, without which removal will only be allowed if
   no tablets are configured to use the removed data dir.

Change-Id: Iacb650115bcdf055542ef2777641a3201fc8e30b
---
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
M src/kudu/fs/block_manager_util.h
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/tools/tool_action_fs.cc
10 files changed, 343 insertions(+), 90 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iacb650115bcdf055542ef2777641a3201fc8e30b
Gerrit-Change-Number: 8978
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2202: support for removing data directories (take two)

[Adar Dembo (Code Review)]

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

Change subject: KUDU-2202: support for removing data directories (take two)
..


Patch Set 2:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:

http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/data_dirs.h@220
PS1, Line 220: the DataDirManager is
 :   // opened as per the new contents of of the provided 
directories but
> nit: extra "of"
Changed to:

  // If 'update_on_disk' and 'read_only' are both true, the directory manager
  // allows the provided directories to deviate from their on-disk data
  // structures without updating the latter to match the former.


http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/data_dirs.h@223
PS1, Line 223:   bool update_on_disk;
> nit: should still note the default
Done


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

http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/data_dirs.cc@657
PS1, Line 657: Note: If 'has_missing_instance' is true, opts_.update_on_disk is
 :   // guaranteed to be true.
> nit: Maybe note that this is due to the ternary parameterization of CheckIn
Hmm, I originally included this so that it isn't confusing that L659-660 omits 
a check for opts_.update_on_disk.

I'll change it as per your first suggestion.


http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/data_dirs.cc@805
PS1, Line 805: // This uuid's directory is missing outright, which can happen 
when
 : // opts_.read_only and opts_.update_on_disk are both 
true.
> After another look, I think this is ok.
OK, I took another stab at this comment. Let me know what you think.


http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:

http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/fs_manager.h@102
PS1, Line 102:   bool update_on_disk;
> nit: note the default
Done


http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/tools/kudu-tool-test.cc@2232
PS1, Line 2232: ASSERT_OK(mts->WaitStarted());
> Could we check that we still have all the tablets we expected?
We could, but is it necessary? We wouldn't expect tablets to just disappear, 
right? We don't check them after adding a data directory above.


http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/tools/kudu-tool-test.cc@2240
PS1, Line 2240:   // Reconfigure the tserver to drop the data directory and 
restart it.
> nit: mind adding to the comment something along the lines of, "waiting for
Yeah, it's not intuitive that WaitStarted will return the first bootstrap 
failure. I'll note that.


http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/tools/tool_action_fs.cc
File src/kudu/tools/tool_action_fs.cc:

http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/tools/tool_action_fs.cc@854
PS1, Line 854: "Updates the set of data directories in an existing Kudu 
filesystem")
 :   .ExtraDescription("If a data directory is in use by a 
tablet and is "
 :   "removed, the operation will fail unless --force is 
also used")
 :   .AddOptionalParameter("fs_wal_dir")
 :   .AddOptionalParameter("fs_data_dirs")
 :   .AddOptionalParameter("force", boost::none, string("If 
true, permits "
 :   "the removal of a data directory that is configured 
for use by "
 :   "existing tablets. Those tablets will fail the next 
time the server "
 :   "is started"))
> micro-nit: I know in docs we favor being explicit, that local replicas are
I'm not sure; as you can see, List uses 'tablets' instead of 'local replicas'.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacb650115bcdf055542ef2777641a3201fc8e30b
Gerrit-Change-Number: 8978
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 11 Jan 2018 00:13:40 +
Gerrit-HasComments: Yes

[kudu-CR] fs: update default data dir group size

[Andrew Wong (Code Review)]

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

Change subject: fs: update default data dir group size
..


Patch Set 2:

Interesting that this change is causing build failures of the randomized disk 
failure test. I think the lower group size is yielding fewer fsyncs for the 
tablet, since it's just not tripping the fault injection.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2dd0d3f8cf140f684318c0dffb45a1091302ecdc
Gerrit-Change-Number: 8995
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 10 Jan 2018 23:11:56 +
Gerrit-HasComments: No

[kudu-CR] fs: update default data dir group size

[Andrew Wong (Code Review)]

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

Change subject: fs: update default data dir group size
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8995/2/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:

http://gerrit.cloudera.org:8080/#/c/8995/2/src/kudu/fs/data_dirs.cc@73
PS2, Line 73: TAG_FLAG(fs_target_data_dirs_per_tablet, evolving);
> Not stable?
I'm looking at flag_tags.h and I think this could fall under "not yet locked 
down", particularly since we're still adjusting defaults. Once we get some more 
mileage, I'll be happy to change it.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2dd0d3f8cf140f684318c0dffb45a1091302ecdc
Gerrit-Change-Number: 8995
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 10 Jan 2018 22:54:09 +
Gerrit-HasComments: Yes

[kudu-CR] data dirs: fix logging message

[Adar Dembo (Code Review)]

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

Change subject: data_dirs: fix logging message
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8135e0ad2411524fcb74b3e349bea7a5828a4237
Gerrit-Change-Number: 8999
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 10 Jan 2018 22:50:20 +
Gerrit-HasComments: No

[kudu-CR] data dirs: fix logging message

[Andrew Wong (Code Review)]

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

Change subject: data_dirs: fix logging message
..


Patch Set 1:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8999/1//COMMIT_MSG@10
PS1, Line 10:
> Nit: got an extra space here
Done


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

http://gerrit.cloudera.org:8080/#/c/8999/1/src/kudu/fs/data_dirs.cc@834
PS1, Line 834:   LOG(INFO) << Substitute(msg);
> Why INFO and not WARNING?
At first thought, WARNING didn't seem appropriate because this doesn't 
necessarily point to anything "wrong". E.g. if the default were 3, it would  
spew warnings by default in clusters configured with 1 data dir.

I suppose it could indicate a improper configuration, but given the upcoming 
new default behavior, I thought INFO would be more appropriate.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8135e0ad2411524fcb74b3e349bea7a5828a4237
Gerrit-Change-Number: 8999
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 10 Jan 2018 22:45:22 +
Gerrit-HasComments: Yes

[kudu-CR] data dirs: fix logging message

[Andrew Wong (Code Review)]

Hello Kudu Jenkins, Adar Dembo,

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

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

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

Change subject: data_dirs: fix logging message
..

data_dirs: fix logging message

If fs_target_data_dirs_per_tablet is set to be greater than the number
of available directories, it will log a message that is dependent on
there being a configured metrics entity, which is not always available.
This patch avoids the potential nullptr access.

This patch also changes the logging to INFO-level instead of
WARNING-level, as the message doesn't necessarily indicate a problem.

Change-Id: I8135e0ad2411524fcb74b3e349bea7a5828a4237
---
M src/kudu/fs/data_dirs.cc
1 file changed, 8 insertions(+), 4 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8135e0ad2411524fcb74b3e349bea7a5828a4237
Gerrit-Change-Number: 8999
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] data dirs: fix logging message

[Adar Dembo (Code Review)]

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

Change subject: data_dirs: fix logging message
..


Patch Set 1:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8999/1//COMMIT_MSG@10
PS1, Line 10:
Nit: got an extra space here


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

http://gerrit.cloudera.org:8080/#/c/8999/1/src/kudu/fs/data_dirs.cc@834
PS1, Line 834:   LOG(INFO) << Substitute(msg);
Why INFO and not WARNING?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8135e0ad2411524fcb74b3e349bea7a5828a4237
Gerrit-Change-Number: 8999
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 10 Jan 2018 22:29:30 +
Gerrit-HasComments: Yes

[kudu-CR] data dirs: fix logging message

[Andrew Wong (Code Review)]

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


Change subject: data_dirs: fix logging message
..

data_dirs: fix logging message

If fs_target_data_dirs_per_tablet is set to be greater than the number
of available directories, it will log a message  that is dependent on
there being a configured metrics entity, which is not always available.
This patch avoids the potential nullptr access.

Change-Id: I8135e0ad2411524fcb74b3e349bea7a5828a4237
---
M src/kudu/fs/data_dirs.cc
1 file changed, 8 insertions(+), 4 deletions(-)



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

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

[kudu-CR] [tls socket-test] fix test to run smoothly at kernel 4.4.x

[Alexey Serbin (Code Review)]

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


Change subject: [tls_socket-test] fix test to run smoothly at kernel 4.4.x
..

[tls_socket-test] fix test to run smoothly at kernel 4.4.x

Change-Id: I94170d5bdbe17a5952a82faf74d8b868b42460aa
---
M src/kudu/security/tls_socket-test.cc
1 file changed, 1 insertion(+), 1 deletion(-)



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

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

[kudu-CR] fs: update default data dir group size

[Andrew Wong (Code Review)]

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


Change subject: fs: update default data dir group size
..

fs: update default data dir group size

This patch makes fs_target_data_dirs_per_tablet non-experimental and
sets the new default to 3. Upon experimenting with the flag via YCSB
workloads, only read workload tail latency seemed to be affected at very
small group sizes (e.g. 1). 3 seems like a reasonable choice, and we can
always update it in the future.

Note that existing data will not be updated with the new flag. Only new
tablets will honor the sizing.

Change-Id: I2dd0d3f8cf140f684318c0dffb45a1091302ecdc
---
M src/kudu/fs/data_dirs.cc
1 file changed, 7 insertions(+), 7 deletions(-)



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

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

[kudu-CR] [webui] Make tombstone tablet info less confusing

[Will Berkeley (Code Review)]

Hello Alexey Serbin, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: [webui] Make tombstone tablet info less confusing
..

[webui] Make tombstone tablet info less confusing

Previously, when a tombstone tablet was reloaded at server startup,
the last status message was "Tablet initializing...". This was
confusing as it set the expectation that something more was going
to happen to the tombstoned tablet. The message is now simply
"Tombstoned".

Also, now that tombstoned tablets can vote, they retain cmeta
despite not participating in non-election Raft operations.
Their list of peers is not updated and not usually relevant. It
might be confusing to see it on the web ui. This patch suppresses
it.

Change-Id: I5c879cc7ff634e5b434fc33374d3010cf1f262cb
---
M src/kudu/tablet/tablet_replica.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/tserver_path_handlers.cc
4 files changed, 56 insertions(+), 6 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5c879cc7ff634e5b434fc33374d3010cf1f262cb
Gerrit-Change-Number: 8981
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] [webui] Make tombstone tablet info less confusing

[Will Berkeley (Code Review)]

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

Change subject: [webui] Make tombstone tablet info less confusing
..


Patch Set 2:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/8981/2/src/kudu/tserver/tserver_path_handlers.cc@211
PS2, Line 211:   return 
replica->HumanReadableState().find("TABLET_DATA_TOMBSTONED") != string::npos;
> hrm, we can't get at this in a less stringy way?
Done


http://gerrit.cloudera.org:8080/#/c/8981/2/src/kudu/tserver/tserver_path_handlers.cc@327
PS2, Line 327: Do not delete them
> think it would be nice to say something like "The tombstone markers are nec
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5c879cc7ff634e5b434fc33374d3010cf1f262cb
Gerrit-Change-Number: 8981
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Wed, 10 Jan 2018 20:33:18 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2202: support for removing data directories (take two)

[Andrew Wong (Code Review)]

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

Change subject: KUDU-2202: support for removing data directories (take two)
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8978/1/src/kudu/fs/data_dirs.cc@805
PS1, Line 805: // This uuid's directory is missing outright, which can happen 
when
 : // opts_.read_only and opts_.update_on_disk are both 
true.
> So this can only happen when we open our "staging" FsManager and we're miss
After another look, I think this is ok.

Since we're only opening up the speculative FsManager, we shouldn't need to 
worry about potential mismatched UUIDs (e.g. if there's a missing directory AND 
a failed directory, we might end up assigning the failed directory the wrong 
UUID).

When we open the FsManager up for real, this shouldn't be the case because at 
that point, we'll update the path_sets and shouldn't reach this code block. Can 
you add a comment explaining why this missing directory doesn't matter?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacb650115bcdf055542ef2777641a3201fc8e30b
Gerrit-Change-Number: 8978
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 10 Jan 2018 19:57:28 +
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,

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

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

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

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,
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/scan_batch.cc
M src/kudu/client/scan_batch.h
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/CMakeLists.txt
M src/kudu/common/common.proto
A src/kudu/common/decimal.cc
A src/kudu/common/decimal.h
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/consensus/log-test.cc
M src/kudu/integration-tests/all_types-itest.cc
M src/kudu/util/int128-test.cc
M src/kudu/util/int128.h
25 files changed, 665 insertions(+), 140 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8830/8
--
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: 8
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