[kudu-CR] WIP [non voter-itest] stress test for the 3-4-3 replacement scheme

[Alexey Serbin (Code Review)]

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


Change subject: WIP [non_voter-itest] stress test for the 3-4-3 replacement 
scheme
..

WIP [non_voter-itest] stress test for the 3-4-3 replacement scheme

Using this test, I was able to hit the following issue:

F0208 05:35:22.152496  2721 ts_tablet_manager.cc:563] T 
5384471d823e46929029f9ff6ce212a3 P c713ac498df040caa897d3229214baa3: Tablet 
Copy: Found tablet in TABLET_DATA_COPYING state during StartTabletCopy()
*** Check failure stack trace: ***
@ 0x7f22d7fa92fd  google::LogMessage::Fail() at ??:0
@ 0x7f22d7fab1bd  google::LogMessage::SendToLog() at ??:0
@ 0x7f22d7fa8e39  google::LogMessage::Flush() at ??:0
@ 0x7f22d7fabc5f  google::LogMessageFatal::~LogMessageFatal() at ??:0
@ 0x7f22e1fa6ed7  kudu::tserver::TSTabletManager::RunTabletCopy() at 
??:0
@ 0x7f22e1fb296f  kudu::tserver::TabletCopyRunnable::Run() at ??:0
@ 0x7f22d9c9a1f5  kudu::ThreadPool::DispatchThread() at ??:0
@ 0x7f22d9ca9069  boost::_mfi::mf0<>::operator()() at ??:0
@ 0x7f22d9ca8fd0  boost::_bi::list1<>::operator()<>() at ??:0
@ 0x7f22d9ca8f7a  boost::_bi::bind_t<>::operator()() at ??:0
@ 0x7f22d9ca8d5d  
boost::detail::function::void_function_obj_invoker0<>::invoke() at ??:0
@ 0x7f22db98d018  boost::function0<>::operator()() at ??:0
@ 0x7f22d9c8bdfd  kudu::Thread::SuperviseThread() at ??:0
@ 0x7f22ddc68184  start_thread at ??:0

WIP: Fix the issue and verify the fix.  Probably, the dist-test machines
filled up because I ran this test: both the tablet servers and the master
were very chatty before I added the --minloglevel=2 option.

Change-Id: I036a882f7e9132a5c26013227c50c0699b59ed6e
---
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/integration-tests/cluster_verifier.h
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
6 files changed, 202 insertions(+), 1 deletion(-)



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

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

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

[Hao Hao (Code Review)]

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

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


Patch Set 11:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto
File src/kudu/common/common.proto:

http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@222
PS10, Line 222: tion is
> nit: snapshot timestamp
Done


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@228
PS10, Line 228:
> nit: choose
Done


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@229
PS10, Line 229:  serv
> nit: such that
Done


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@231
PS10, Line 231:
> The 'stale' terminology hasn't been introduced here, so I think it would be
Done


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/common/common.proto@233
PS10, Line 233:
> I think it would be better to say 'different results' here in order to avoi
Done


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

http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1922
PS10, Line 1922:
> We decided we also wanted scan your reads right? is that covered?
Done


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1944
PS10, Line 1944:   ASSERT_EQ(R"((in
> I know this is likely copy/paste but remove (adds nothing)
Done


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1953
PS10, Line 1953:   ASSERT_EQ(kNumRows, results.size());
   :   ASSERT_EQ(R"((int32 key=0, int32 int_val
> I think we discussed a couple of reasons why it might be interesting to hav
Right, updated it accordingly.


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_server-test.cc@1989
PS10, Line 1989: Hybrid
> same
Done


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.h
File src/kudu/tserver/tablet_service.h:

http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.h@158
PS10, Line 158: ing to the scan m
> on the read mode?
Done


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2107
PS10, Line 2107: us s = server_->cl
> nit add a predict false
Done


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2161
PS10, Line 2161: )
> use kMinTimestamp or something? I understand you can't use kInvalidTimestam
Done


http://gerrit.cloudera.org:8080/#/c/8804/10/src/kudu/tserver/tablet_service.cc@2163
PS10, Line 2163: Timestamp(std::max(propagated_timestamp + 1, clean_timestamp));
   : RETURN_NOT_OK(ValidateTimeStamp(tmp_snap_timestamp));
> std:max ?
Done



--
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: comment
Gerrit-Change-Id: I84ddb981a1a0f199d4e66f5d5097318f8c785a48
Gerrit-Change-Number: 8804
Gerrit-PatchSet: 11
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Thu, 08 Feb 2018 04:36:00 +
Gerrit-HasComments: Yes

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

[Hao Hao (Code Review)]

Hello Alexey Serbin, Mike Percy, Dan Burkert, 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 (#11).

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_YOUR_WRITES mode is not
repeatable. However, it allows client local
read-your-writes/read-your-reads.

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, 234 insertions(+), 52 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/11
--
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: 11
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] KUDU-2291 (part 1): allow collecting a thread's stack without immediate symbolization

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

to review the following change.


Change subject: KUDU-2291 (part 1): allow collecting a thread's stack without 
immediate symbolization
..

KUDU-2291 (part 1): allow collecting a thread's stack without immediate 
symbolization

For the /stacks page we would like to be able to grab the stack from a
bunch of threads as quickly as possible to eliminate "skid" between the
collection times. Symbolization of stacks is much slower than
collection, so this patch adds a new API to collect the remote thread
stack without symbolizing it immediately.

This adds a simple benchmark for remote thread stack collection. The
initial results are not that different between symbolized and
unsymbolized because the stack collection performance itself is poor.
This will be improved in a later patch in this series.

I0207 19:33:20.960477 30799 debug-util-test.cc:175] Throughput: 99 dumps/second 
(symbolize=0)
I0207 19:33:21.963946 30799 debug-util-test.cc:175] Throughput: 84 dumps/second 
(symbolize=1)

Change-Id: I0a14fbb7d0cafcd7b966dac507e71bae32e2cc1a
---
M src/kudu/util/debug-util-test.cc
M src/kudu/util/debug-util.cc
M src/kudu/util/debug-util.h
3 files changed, 51 insertions(+), 9 deletions(-)



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

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

[kudu-CR] WIP: KUDU-2291 (part 2): Add a /stacks page

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

to review the following change.


Change subject: WIP: KUDU-2291 (part 2): Add a /stacks page
..

WIP: KUDU-2291 (part 2): Add a /stacks page

This adds a simple /stacks web page which dumps the currently running
threads in a plain text format.

Since we often have a lot of idle threadpool threads sitting around in
the "wait for work" state, the output collapses all threads with
matching stacks and only displays the stack once, making it more
suitable for human consumption.

Example output from a local kudu-master:
  https://gist.github.com/b64739ee5fb146ea1953380f57b996c4

WIP since maybe this should be consolidated with /threadz?

Change-Id: I8b8f6d50d44e40fd51357fdbfd8f9ba2ebaa724b
---
M src/kudu/server/default_path_handlers.cc
1 file changed, 88 insertions(+), 0 deletions(-)



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

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

[kudu-CR] KUDU-2291 (part 3): use futex to speed up stack collection

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

to review the following change.


Change subject: KUDU-2291 (part 3): use futex to speed up stack collection
..

KUDU-2291 (part 3): use futex to speed up stack collection

Previously the thread which requests a stack trace would poll on an
atomic variable waiting for the stack to be collected. Of course
spinning is too CPU-hungry so it would actually sleep for 10ms in
each iteration. This limited the performance of stack collection to
about 100/second, which is problematic for the /stacks web page on a
server that may have thousands of threads.

This switches to using the futex system call to allow the dumper thread
to sleep and the dumpee thread to wake it back up when the results are
ready.

Why do we use the low level futex and not something like a condition
variable or CountdownLatch? The issue is that the dumpee thread is
running in a signal handler context, and most things are not safe to do
in that context. Particularly, mutexes may block, which is very naughty,
and condition variables and latches are currently implemented on top of
mutexes. pthread semaphores are another option, but those don't support
proper timeout semantics, which is a feature we use in this use case.

Whether the futex syscall itself is async-signal-safe is up for some debate.
Clearly the "wait" mode is not, since it may block, but is the "wake" mode
that we use here allowed? Technically I don't think so. In practice,
the pthread sem_post() API _is_ documented to be async-signal-safe and
I verified that in libc source it's implemented using futex to wake the
waiter. So, if we are doing something illegal, at least we're doing
the same illegal thing as libc, so we're very unlikely to ever break.

Obviously futex is not portable to macOS, but this code was already
Linux-specific so we haven't lost much.

This speeds up the benchmark for stack trace collection (without symbolization)
by 585x:

Before:
  I0207 19:33:20.960477 30799 debug-util-test.cc:175] Throughput: 99 
dumps/second (symbolize=0)
  I0207 19:33:21.963946 30799 debug-util-test.cc:175] Throughput: 84 
dumps/second (symbolize=1)

After:
  I0207 19:38:32.505213 32039 debug-util-test.cc:175] Throughput: 57916 
dumps/second (symbolize=0)
  I0207 19:38:33.506417 32039 debug-util-test.cc:175] Throughput: 595 
dumps/second (symbolize=1)

Change-Id: Icb772f0607275ec340c7f38be05347fc14820cbd
---
M src/kudu/util/debug-util.cc
1 file changed, 63 insertions(+), 15 deletions(-)



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

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

[kudu-CR] Add back KuduColumnSchema DataTypeToString

[Grant Henke (Code Review)]

Hello Tidy Bot, Dan Burkert, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: Add back KuduColumnSchema DataTypeToString
..

Add back KuduColumnSchema DataTypeToString

KuduColumnSchema’s DataTypeToString method
was accidentally removed in KUDU-721.

This method/functionality should be added back
to prevent a compatibility break. However, it’s
implimentation has been updated to print based
on the public DataType instead of converting to the
internal type and printing that.

Change-Id: I2bf6879f5ab623366a962c6a3d51efc49978a53c
---
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
2 files changed, 34 insertions(+), 1 deletion(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2bf6879f5ab623366a962c6a3d51efc49978a53c
Gerrit-Change-Number: 9240
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add back KuduColumnSchema DataTypeToString

[Grant Henke (Code Review)]

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

Change subject: Add back KuduColumnSchema DataTypeToString
..


Patch Set 2:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9240/2/src/kudu/client/schema.cc@568
PS2, Line 568:   return DataType_Name(ToInternalDataType(type, 
type_attributes));
> Isn't this going to return 'Decimal32'/'Decimal64'/'Decimal128'?  I don't t
Yeah, I was mainly trying to maintain existing functionality because I didn't 
fully understand the existing implementation. Before decimal, I am not sure why 
the old implementation called ToInternalDataType on the public data type and 
used DataType_Name. My best guess now that I look deeper is that this was the 
"easiest" way to get a string from the public data type since protobuf 
generated a function for it. That seams a bit contrived though.

Instead I can change DataTypeToString to simply output a string for each public 
DataType enum entry. Is there a more clever method than an array of strings in 
c++?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bf6879f5ab623366a962c6a3d51efc49978a53c
Gerrit-Change-Number: 9240
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 08 Feb 2018 02:45:15 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2274. consensus: Remove ConsensusMetadata thread safety

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9246 )

Change subject: KUDU-2274. consensus: Remove ConsensusMetadata thread safety
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9246/1/src/kudu/consensus/consensus_meta.h
File src/kudu/consensus/consensus_meta.h:

PS1:
> Is this just a WIP patch?
Well, semi WIP, I wanted your opinion but I tend to agree that we should 
restructure the class and use a recursive fake mutex (per the commit message).


http://gerrit.cloudera.org:8080/#/c/9246/1/src/kudu/consensus/consensus_meta.h@236
PS1, Line 236: TODO(mpercy) Doc this
> Indeed: it would be nice to explain the reasoning behind having this fake m
oops, will do


http://gerrit.cloudera.org:8080/#/c/9246/1/src/kudu/consensus/consensus_meta.h@237
PS1, Line 237: lock_
> nit: maybe, rename it to fake_lock_?
good idea



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8d086c3fba52826ebe0d3a44842d53ecb6a9265
Gerrit-Change-Number: 9246
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Wed, 07 Feb 2018 23:04:00 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: [Spark] Add DECIMAL type support

[Grant Henke (Code Review)]

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

Change subject: KUDU-721: [Spark] Add DECIMAL type support
..


Patch Set 2:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9213/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@308
PS2, Line 308: _: DecimalType
> Should be able to do
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5f7a801778ed81b68949bbf8d7c08d1a13ed840
Gerrit-Change-Number: 9213
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 07 Feb 2018 23:03:04 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2274. consensus: Remove ConsensusMetadata thread safety

[Alexey Serbin (Code Review)]

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

Change subject: KUDU-2274. consensus: Remove ConsensusMetadata thread safety
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9246/1/src/kudu/consensus/consensus_meta.h
File src/kudu/consensus/consensus_meta.h:

PS1:
> Well, semi WIP, I wanted your opinion but I tend to agree that we should re
Yep, overall it LGTM.  I think it's a movement in the right direction.  By my 
opinion, once converted into non-thread safe with fake mutex for additional 
consistency, this class needs a bit of restructuring and cleanup.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8d086c3fba52826ebe0d3a44842d53ecb6a9265
Gerrit-Change-Number: 9246
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Comment-Date: Wed, 07 Feb 2018 23:12:48 +
Gerrit-HasComments: Yes

[kudu-CR] Fix unlocked access to cmeta in RaftConsensus::Start()

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9245 )

Change subject: Fix unlocked access to cmeta in RaftConsensus::Start()
..

Fix unlocked access to cmeta in RaftConsensus::Start()

In a future patch, after removing the thread safety of the
ConsensusMetadata class, unlocked access to cmeta triggered a warning
from the collision warner in RaftConsensus::Start():

  F0206 20:31:24.554322 30972 thread_collision_warner.cc:23] Thread Collision! 
Previous thread id: 30962, current thread id: 30972

  Thread 4331 (Thread 0x7ffe5f964700 (LWP 30962)):
  #0  pthread_cond_timedwait@@GLIBC_2.3.2 () at 
../sysdeps/unix/sysv/linux/x86_64/pthread_cond_timedwait.S:225
  #1  0x7331966f in kudu::ConditionVariable::TimedWait (this=0x8c10b0, 
max_time=...) at ../../src/kudu/util/condition_variable.cc:123
  #2  0x774e6c71 in kudu::tserver::Heartbeater::Thread::RunThread 
(this=0x8c0f80) at ../../src/kudu/tserver/heartbeater.cc:538
  #3  0x774efec9 in boost::_mfi::mf0::operator() (this=0xd015d0, p=0x8c0f80) at 
../../thirdparty/installed/uninstrumented/include/boost/bind/mem_fn_template.hpp:49

  Thread 4341 (Thread 0x7ffe5a95a700 (LWP 30972)):
  #0  0x7156c428 in __GI_raise (sig=sig@entry=6) at 
../sysdeps/unix/sysv/linux/raise.c:54
  #1  0x7156e02a in __GI_abort () at abort.c:89
  #2  0x72ff1c67 in google::DumpStackTraceAndExit () at 
/home/mpercy/src/kudu/thirdparty/src/glog-0.3.5/src/utilities.cc:152
  #3  0x72fe8b1d in google::LogMessage::Fail () at 
/home/mpercy/src/kudu/thirdparty/src/glog-0.3.5/src/logging.cc:1488
  #4  0x72feaa03 in google::LogMessage::SendToLog (this=0x7ffe5a957d68) 
at /home/mpercy/src/kudu/thirdparty/src/glog-0.3.5/src/logging.cc:1442
  #5  0x72fe867a in google::LogMessage::Flush 
(this=this@entry=0x7ffe5a957d68) at 
/home/mpercy/src/kudu/thirdparty/src/glog-0.3.5/src/logging.cc:1311
  #6  0x72feb3cf in google::LogMessageFatal::~LogMessageFatal 
(this=0x7ffe5a957d68, __in_chrg=) at 
/home/mpercy/src/kudu/thirdparty/src/glog-0.3.5/src/logging.cc:2023
  #7  0x730bc20b in base::DCheckAsserter::warn (this=0x7fffbc016a90, 
previous_thread_id=30962, current_thread_id=30972) at 
../../src/kudu/gutil/threading/thread_collision_warner.cc:23
  #8  0x730bc34d in base::ThreadCollisionWarner::Enter 
(this=0x7fffbc00bde0) at 
../../src/kudu/gutil/threading/thread_collision_warner.cc:81
  #9  0x76dd3933 in 
base::ThreadCollisionWarner::ScopedCheck::ScopedCheck (this=0x7ffe5a957e30, 
warner=0x7fffbc00bde0) at 
../../src/kudu/gutil/threading/thread_collision_warner.h:184
  #10 0x76a95fca in kudu::consensus::ConsensusMetadata::current_term 
(this=0x7fffbc00bd90) at ../../src/kudu/consensus/consensus_meta.cc:56
  #11 0x76afa4a4 in 
kudu::consensus::RaftConsensus::Start(kudu::consensus::ConsensusBootstrapInfo 
const&, gscoped_ptr, 
scoped_refptr, scoped_refptr, 
kudu::consensus::ReplicaTransactionFactory*, scoped_refptr, 
kudu::Callback, 
std::allocator > const&)>) (this=0x7fffbc0008d0, info=..., 
peer_proxy_factory=..., log=..., time_manager=..., txn_factory=0x7fffbc00bff0, 
metric_entity=..., mark_dirty_clbk=...) at 
../../src/kudu/consensus/raft_consensus.cc:228
  #12 0x76e231b5 in kudu::tablet::TabletReplica::Start 
(this=0x7fffbc00bff0, bootstrap_info=..., tablet=std::shared_ptr (empty) 0x0, 
clock=..., messenger=std::shared_ptr (empty) 0x0, result_tracker=..., log=..., 
prepare_pool=0xd07140) at ../../src/kudu/tablet/tablet_replica.cc:220

Change-Id: I661c603a57b9ecaeee926ce7cd86c9ecf2ad58a8
Reviewed-on: http://gerrit.cloudera.org:8080/9245
Reviewed-by: Alexey Serbin 
Tested-by: Kudu Jenkins
---
M src/kudu/consensus/raft_consensus.cc
1 file changed, 1 insertion(+), 1 deletion(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I661c603a57b9ecaeee926ce7cd86c9ecf2ad58a8
Gerrit-Change-Number: 9245
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] Add back KuduColumnSchema DataTypeToString

[Dan Burkert (Code Review)]

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

Change subject: Add back KuduColumnSchema DataTypeToString
..


Patch Set 2:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/9240/2/src/kudu/client/schema.h@204
PS2, Line 204:   static std::string DataTypeToString(DataType type, 
KuduColumnTypeAttributes type_attributes);
KuduColumnTypeAttributes doesn't have a move or copy constructor, so can this 
even be called?  Not sure how that works.


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

http://gerrit.cloudera.org:8080/#/c/9240/2/src/kudu/client/schema.cc@568
PS2, Line 568:   return DataType_Name(ToInternalDataType(type, 
type_attributes));
Isn't this going to return 'Decimal32'/'Decimal64'/'Decimal128'?  I don't think 
we should expose that - the underlying storage size is entirely an 
implementation detail.  Returning something like 'Decimal(2, 2)' would be 
better. Also, tests would be nice.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bf6879f5ab623366a962c6a3d51efc49978a53c
Gerrit-Change-Number: 9240
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 07 Feb 2018 23:12:22 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: [Spark] Add DECIMAL type support

[Grant Henke (Code Review)]

Hello Dan Burkert, Kudu Jenkins, Hao Hao,

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

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

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

Change subject: KUDU-721: [Spark] Add DECIMAL type support
..

KUDU-721: [Spark] Add DECIMAL type support

Adds DECIMAL support to the Kudu Spark source and RDD.

Change-Id: Ia5f7a801778ed81b68949bbf8d7c08d1a13ed840
---
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M 
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
M 
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
6 files changed, 84 insertions(+), 25 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia5f7a801778ed81b68949bbf8d7c08d1a13ed840
Gerrit-Change-Number: 9213
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Add previous / current thread to collision warner

[Mike Percy (Code Review)]

Mike Percy has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9237 )

Change subject: Add previous / current thread to collision warner
..

Add previous / current thread to collision warner

The failure message is now more useful and looks like the following:

F0206 20:31:24.554322 30972 thread_collision_warner.cc:23] Thread Collision! 
Previous thread id: 30962, current thread id: 30972

I found this necessary when debugging a particular issue since by the
time the SIGABRT triggered in GDB the previous thread had already exited
the critical section (perhaps while printing the fatal log message).

Change-Id: I038bad901b5235725084bd8671ef4b02ec22c0a7
Reviewed-on: http://gerrit.cloudera.org:8080/9237
Reviewed-by: Alexey Serbin 
Tested-by: Kudu Jenkins
---
M src/kudu/gutil/threading/thread_collision_warner.cc
M src/kudu/gutil/threading/thread_collision_warner.h
2 files changed, 18 insertions(+), 15 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I038bad901b5235725084bd8671ef4b02ec22c0a7
Gerrit-Change-Number: 9237
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] WIP: Create a rolling-failure endurance test

[Alexey Serbin (Code Review)]

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

Change subject: WIP: Create a rolling-failure endurance test
..


Patch Set 3:

(3 comments)

Looks good and this script worked for me, thanks!  Just some nits.

http://gerrit.cloudera.org:8080/#/c/9222/3/src/kudu/scripts/rolling-failure-test.pl
File src/kudu/scripts/rolling-failure-test.pl:

http://gerrit.cloudera.org:8080/#/c/9222/3/src/kudu/scripts/rolling-failure-test.pl@28
PS3, Line 28: that it has passwordless sudo access on the local machine,
: # that the "kudu" administrative tool is in the local PATH, that 
it has
: # passwordless sudo access on every machine on the cluster (after 
ssh'ing into
: # each machine), and that the starting state is a healthy cluster 
with all
: # daemons running
nit: maybe, convert this into a bullet list for better readability?


http://gerrit.cloudera.org:8080/#/c/9222/3/src/kudu/scripts/rolling-failure-test.pl@154
PS3, Line 154: ulimit -Hu 59671; ulimit -Su 59671;
I'm not sure it's worth specifying -H and -S here, since the doc page on bash 
(the ulimit section) says:

  When setting new limits, if neither `-H' nor `-S' is supplied, both the hard 
and soft limits are set.

Also, what if setting limits fails?  Maybe, replace semi-column ';' with 
ampersand '&' to address that?


http://gerrit.cloudera.org:8080/#/c/9222/3/src/kudu/scripts/rolling-failure-test.pl@158
PS3, Line 158: ulimit -u 59671
Is this needed here as well?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib47547f06c0f490a1798990b46d3156c583320fc
Gerrit-Change-Number: 9222
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 07 Feb 2018 21:39:30 +
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 10:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8692/6/src/kudu/hms/hms_client-test.cc@247
PS6, Line 247:
does it fail gracefully or does it CHECK? would be good to ensure that we don't 
crash kudu if we run against a misbehaving hms


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

http://gerrit.cloudera.org:8080/#/c/8692/10/src/kudu/hms/sasl_client_transport.cc@111
PS10, Line 111:   // is complete the negotiated value will be used.
does HMS know how to interpret split frames? I don't see any reason we would 
send a thrift message >64kb for our use cases but I wonder if the error would 
be completely impossible to understand if we did


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

http://gerrit.cloudera.org:8080/#/c/8692/10/src/kudu/rpc/sasl_common.cc@461
PS10, Line 461: switch (val) {
nit: indentation



--
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: 10
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, 07 Feb 2018 20:52:14 +
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 6:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8692/6/src/kudu/hms/hms_client-test.cc@247
PS6, Line 247: fails
> does it fail gracefully or does it CHECK? would be good to ensure that we d
oops, accidentally published this comment on earlier rev. ignore.



--
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: 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 
Gerrit-Comment-Date: Wed, 07 Feb 2018 20:52:39 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2274. WIP: Shut down tombstoned replica when replacing it

[Alexey Serbin (Code Review)]

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

Change subject: KUDU-2274. WIP: Shut down tombstoned replica when replacing it
..


Patch Set 2: Code-Review+1

I think this looks good as is, but of course it would be nice to get a test to 
trigger the issue.  I tried my test scenario posted as a WIP patch at 
https://gerrit.cloudera.org/#/c/9116/ against this patch (slightly modifed, 
just commented out old_replica->Shutdown() in 
TSTabletManager::RunTabletCopy()), but that didn't hit the CHECK() added in 
this patch.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5508b70946c9341456407c12551b19eba488d191
Gerrit-Change-Number: 9156
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 07 Feb 2018 20:48:28 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2274. consensus: Remove ConsensusMetadata thread safety

[Alexey Serbin (Code Review)]

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

Change subject: KUDU-2274. consensus: Remove ConsensusMetadata thread safety
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9246/1/src/kudu/consensus/consensus_meta.h
File src/kudu/consensus/consensus_meta.h:

PS1:
Is this just a WIP patch?

There are multiple methods in this file called xxx_unlocked() and 
XxxUnlocked().  By my understanding, if it's not a WIP patch, it would be nice 
to get rid of names asserting on thread-safe behavior of this class, removing 
variations of the unlocked methods.

If I'm not mistaken, it's possible to embed methods protected by scoped fake 
lock.


http://gerrit.cloudera.org:8080/#/c/9246/1/src/kudu/consensus/consensus_meta.h@236
PS1, Line 236: TODO(mpercy) Doc this
Indeed: it would be nice to explain the reasoning behind having this fake mutex.


http://gerrit.cloudera.org:8080/#/c/9246/1/src/kudu/consensus/consensus_meta.h@237
PS1, Line 237: lock_
nit: maybe, rename it to fake_lock_?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8d086c3fba52826ebe0d3a44842d53ecb6a9265
Gerrit-Change-Number: 9246
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 07 Feb 2018 20:42:45 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-721: [Spark] Add DECIMAL type support

[Dan Burkert (Code Review)]

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

Change subject: KUDU-721: [Spark] Add DECIMAL type support
..


Patch Set 2:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/9213/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@308
PS2, Line 308: _: DecimalType
Should be able to do

case DecimalType() =>  ...


to make it match better with the other cases.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia5f7a801778ed81b68949bbf8d7c08d1a13ed840
Gerrit-Change-Number: 9213
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 07 Feb 2018 20:24:15 +
Gerrit-HasComments: Yes

[kudu-CR] Fix unlocked access to cmeta in RaftConsensus::Start()

[Alexey Serbin (Code Review)]

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

Change subject: Fix unlocked access to cmeta in RaftConsensus::Start()
..


Patch Set 1: Code-Review+2

Nice find!


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I661c603a57b9ecaeee926ce7cd86c9ecf2ad58a8
Gerrit-Change-Number: 9245
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 07 Feb 2018 20:23:21 +
Gerrit-HasComments: No

[kudu-CR] KUDU-721: [Java] Add DECIMAL column type support

[Dan Burkert (Code Review)]

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

Change subject: KUDU-721: [Java] Add DECIMAL column type support
..


Patch Set 11:

(17 comments)

Similar concerns from the C++ side, I'm not seeing any coverage of PK decimals 
with predicates.

http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java
File java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java:

http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/ColumnSchema.java@331
PS11, Line 331:   this.typeAttributes = typeAttributes;
Might be good to precondition this on the type being decimal.


http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java
File java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java:

http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/ColumnTypeAttributes.java@98
PS11, Line 98: int result = hasPrecision ? 1 : 0;
Use Objects.hash instead of hand-rolling the hash.  There's some allocation 
overhead, but hard coded primes are pretty yucky, and I don't see a high-perf 
usecase for putting these in a hashmap.


http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java:

http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java@618
PS11, Line 618:* @param b The array to write to.
Specify the array must be zeroed.


http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java@631
PS11, Line 631: n.toByteArray()
can this use the 'bytes' array that was already extracted?  May need to switch 
reverseBytes to be in-place.


http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java@656
PS11, Line 656:   private static byte[] reverseBytes(final byte[] b) {
Looks like this would be more efficient as an in-place reverse, everywhere that 
calls it has already made defensive copies.


http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java@833
PS11, Line 833: // TODO: use n.unscaledValue().intValueExact() when we 
drop Java7 support.
It's worth copying in the impl here, since it's short and changing it later 
could be considered backwards incompatible. 
https://github.com/openjdk-mirror/jdk/blob/jdk8u/jdk8u/master/src/share/classes/java/math/BigInteger.java?utf8=%E2%9C%93#L4398-L4403


http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java
File 
java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java:

http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java@41
PS11, Line 41: @Deprecated
This class is deprecated, so you shouldn't add any new APIs to it.


http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java
File java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java:

http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java@1062
PS11, Line 1062:return Bytes.getDecimal(value, 
typeAttributes.getPrecision(), typeAttributes.getScale()).toString();
Wrap this line.


http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java
File java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java:

http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java@306
PS11, Line 306: rows.put(rowData, currentRowOffset, 
col.getType().getSize(col.getTypeAttributes()));
Consider hoisting the size into a local variable now that it's no longer a 
cheap accessor.  This method is called once for each row in a write batch, so I 
think it's perf sensitive.


http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java
File java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java:

http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@529
PS11, Line 529:* Get the specified column's Decimal
add a period.


http://gerrit.cloudera.org:8080/#/c/8882/11/java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java@1240
PS11, Line 1240: if (existing.equals(incremented)) {
The logic here is wrong, it needs to be like the integer types above not the 
float types.  Please make sure there is coverage of this in a decimal primary 
key 

[kudu-CR] docs: update docs for update dirs tool

[Alex Rodoni (Code Review)]

Alex Rodoni has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9110 )

Change subject: docs: update docs for update_dirs tool
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9110/2/docs/administration.adoc
File docs/administration.adoc:

http://gerrit.cloudera.org:8080/#/c/9110/2/docs/administration.adoc@797
PS2, Line 797: In the event that critical files are lost (i.e. WALs or 
tablet-specific
 : metadata), to ensure correctness, all Kudu directories on the 
server must be deleted
 : and rebuilt,
In the event that critical files are lost, i.e. WALS or tablet-specific 
metadata, all Kudu directories on the server must be deleted and rebuilt to 
ensure correctness.


http://gerrit.cloudera.org:8080/#/c/9110/2/docs/administration.adoc@799
PS2, Line 799: destroying
who destroys? The customer or Kudu? Start a new sentence that "... will 
destroy..."



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic3c139326aee35f72495a1cb1aaf8df1d58776cb
Gerrit-Change-Number: 9110
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 07 Feb 2018 20:15:39 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2274. WIP: Shut down tombstoned replica when replacing it

[Mike Percy (Code Review)]

Hello Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: KUDU-2274. WIP: Shut down tombstoned replica when replacing it
..

KUDU-2274. WIP: Shut down tombstoned replica when replacing it

Also improve destructor state checks in TabletReplica.

TODO: Needs a test.

Change-Id: I5508b70946c9341456407c12551b19eba488d191
---
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tserver/ts_tablet_manager.cc
2 files changed, 8 insertions(+), 3 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5508b70946c9341456407c12551b19eba488d191
Gerrit-Change-Number: 9156
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Fix unlocked access to cmeta in RaftConsensus::Start()

[Mike Percy (Code Review)]

Hello Alexey Serbin,

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

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

to review the following change.


Change subject: Fix unlocked access to cmeta in RaftConsensus::Start()
..

Fix unlocked access to cmeta in RaftConsensus::Start()

In a future patch, after removing the thread safety of the
ConsensusMetadata class, unlocked access to cmeta triggered a warning
from the collision warner in RaftConsensus::Start():

  F0206 20:31:24.554322 30972 thread_collision_warner.cc:23] Thread Collision! 
Previous thread id: 30962, current thread id: 30972

  Thread 4331 (Thread 0x7ffe5f964700 (LWP 30962)):
  #0  pthread_cond_timedwait@@GLIBC_2.3.2 () at 
../sysdeps/unix/sysv/linux/x86_64/pthread_cond_timedwait.S:225
  #1  0x7331966f in kudu::ConditionVariable::TimedWait (this=0x8c10b0, 
max_time=...) at ../../src/kudu/util/condition_variable.cc:123
  #2  0x774e6c71 in kudu::tserver::Heartbeater::Thread::RunThread 
(this=0x8c0f80) at ../../src/kudu/tserver/heartbeater.cc:538
  #3  0x774efec9 in boost::_mfi::mf0::operator() (this=0xd015d0, p=0x8c0f80) at 
../../thirdparty/installed/uninstrumented/include/boost/bind/mem_fn_template.hpp:49

  Thread 4341 (Thread 0x7ffe5a95a700 (LWP 30972)):
  #0  0x7156c428 in __GI_raise (sig=sig@entry=6) at 
../sysdeps/unix/sysv/linux/raise.c:54
  #1  0x7156e02a in __GI_abort () at abort.c:89
  #2  0x72ff1c67 in google::DumpStackTraceAndExit () at 
/home/mpercy/src/kudu/thirdparty/src/glog-0.3.5/src/utilities.cc:152
  #3  0x72fe8b1d in google::LogMessage::Fail () at 
/home/mpercy/src/kudu/thirdparty/src/glog-0.3.5/src/logging.cc:1488
  #4  0x72feaa03 in google::LogMessage::SendToLog (this=0x7ffe5a957d68) 
at /home/mpercy/src/kudu/thirdparty/src/glog-0.3.5/src/logging.cc:1442
  #5  0x72fe867a in google::LogMessage::Flush 
(this=this@entry=0x7ffe5a957d68) at 
/home/mpercy/src/kudu/thirdparty/src/glog-0.3.5/src/logging.cc:1311
  #6  0x72feb3cf in google::LogMessageFatal::~LogMessageFatal 
(this=0x7ffe5a957d68, __in_chrg=) at 
/home/mpercy/src/kudu/thirdparty/src/glog-0.3.5/src/logging.cc:2023
  #7  0x730bc20b in base::DCheckAsserter::warn (this=0x7fffbc016a90, 
previous_thread_id=30962, current_thread_id=30972) at 
../../src/kudu/gutil/threading/thread_collision_warner.cc:23
  #8  0x730bc34d in base::ThreadCollisionWarner::Enter 
(this=0x7fffbc00bde0) at 
../../src/kudu/gutil/threading/thread_collision_warner.cc:81
  #9  0x76dd3933 in 
base::ThreadCollisionWarner::ScopedCheck::ScopedCheck (this=0x7ffe5a957e30, 
warner=0x7fffbc00bde0) at 
../../src/kudu/gutil/threading/thread_collision_warner.h:184
  #10 0x76a95fca in kudu::consensus::ConsensusMetadata::current_term 
(this=0x7fffbc00bd90) at ../../src/kudu/consensus/consensus_meta.cc:56
  #11 0x76afa4a4 in 
kudu::consensus::RaftConsensus::Start(kudu::consensus::ConsensusBootstrapInfo 
const&, gscoped_ptr, 
scoped_refptr, scoped_refptr, 
kudu::consensus::ReplicaTransactionFactory*, scoped_refptr, 
kudu::Callback, 
std::allocator > const&)>) (this=0x7fffbc0008d0, info=..., 
peer_proxy_factory=..., log=..., time_manager=..., txn_factory=0x7fffbc00bff0, 
metric_entity=..., mark_dirty_clbk=...) at 
../../src/kudu/consensus/raft_consensus.cc:228
  #12 0x76e231b5 in kudu::tablet::TabletReplica::Start 
(this=0x7fffbc00bff0, bootstrap_info=..., tablet=std::shared_ptr (empty) 0x0, 
clock=..., messenger=std::shared_ptr (empty) 0x0, result_tracker=..., log=..., 
prepare_pool=0xd07140) at ../../src/kudu/tablet/tablet_replica.cc:220

Change-Id: I661c603a57b9ecaeee926ce7cd86c9ecf2ad58a8
---
M src/kudu/consensus/raft_consensus.cc
1 file changed, 1 insertion(+), 1 deletion(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I661c603a57b9ecaeee926ce7cd86c9ecf2ad58a8
Gerrit-Change-Number: 9245
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 

[kudu-CR] KUDU-2274. consensus: Remove ConsensusMetadata thread safety

[Mike Percy (Code Review)]

Hello Alexey Serbin,

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

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

to review the following change.


Change subject: KUDU-2274. consensus: Remove ConsensusMetadata thread safety
..

KUDU-2274. consensus: Remove ConsensusMetadata thread safety

ConsensusMetadata doesn't need to be thread-safe, even though it is
ref-counted, because it is required to be externally synchronized.
This patch replaces the mutex with a DFAKE_MUTEX from the thread
collision warner utility class in order to easily detect concurrent
access due to buggy external sychronization.

This class could be restructured to remove the distinction between
"locked" and "unlocked" methods by using the recursive version of the
thread collision warner in every public method. Perhaps we should do
that instead.

Change-Id: Ia8d086c3fba52826ebe0d3a44842d53ecb6a9265
---
M src/kudu/consensus/consensus_meta.cc
M src/kudu/consensus/consensus_meta.h
2 files changed, 28 insertions(+), 40 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia8d086c3fba52826ebe0d3a44842d53ecb6a9265
Gerrit-Change-Number: 9246
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 

[kudu-CR] Add previous / current thread to collision warner

[Alexey Serbin (Code Review)]

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

Change subject: Add previous / current thread to collision warner
..


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9237/1/src/kudu/gutil/threading/thread_collision_warner.h
File src/kudu/gutil/threading/thread_collision_warner.h:

http://gerrit.cloudera.org:8080/#/c/9237/1/src/kudu/gutil/threading/thread_collision_warner.h@132
PS1, Line 132:
 : // The class ThreadCollisionWarner uses an Asserter to notify 
the collision
 : // AsserterBase is the interfaces and DCheckAsserter is the 
default asserter
 : // used. During the unit tests is used another class that 
doesn't "D
> I'd rather not change that because it's a copy / paste import from Chromium
Ah, that makes sense.  SGTM.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I038bad901b5235725084bd8671ef4b02ec22c0a7
Gerrit-Change-Number: 9237
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 07 Feb 2018 20:03:19 +
Gerrit-HasComments: Yes

[kudu-CR] Add previous / current thread to collision warner

[Mike Percy (Code Review)]

Hello Alexey Serbin, Kudu Jenkins, Todd Lipcon,

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

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

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

Change subject: Add previous / current thread to collision warner
..

Add previous / current thread to collision warner

The failure message is now more useful and looks like the following:

F0206 20:31:24.554322 30972 thread_collision_warner.cc:23] Thread Collision! 
Previous thread id: 30962, current thread id: 30972

I found this necessary when debugging a particular issue since by the
time the SIGABRT triggered in GDB the previous thread had already exited
the critical section (perhaps while printing the fatal log message).

Change-Id: I038bad901b5235725084bd8671ef4b02ec22c0a7
---
M src/kudu/gutil/threading/thread_collision_warner.cc
M src/kudu/gutil/threading/thread_collision_warner.h
2 files changed, 18 insertions(+), 15 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I038bad901b5235725084bd8671ef4b02ec22c0a7
Gerrit-Change-Number: 9237
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [build] fix on ld's -l:path to file notation on OS X

[Todd Lipcon (Code Review)]

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

Change subject: [build] fix on ld's -l:path_to_file notation on OS X
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I65304c2d7c37abdddb8c60eac0e288b546cfc530
Gerrit-Change-Number: 9242
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 07 Feb 2018 19:58:36 +
Gerrit-HasComments: No

[kudu-CR] Add previous / current thread to collision warner

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9237 )

Change subject: Add previous / current thread to collision warner
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9237/1/src/kudu/gutil/threading/thread_collision_warner.h
File src/kudu/gutil/threading/thread_collision_warner.h:

http://gerrit.cloudera.org:8080/#/c/9237/1/src/kudu/gutil/threading/thread_collision_warner.h@132
PS1, Line 132: // The class ThreadCollisionWarner uses an Asserter to notify 
the collision
 : // AsserterBase is the interfaces and DCheckAsserter is the 
default asserter
 : // used. During the unit tests is used another class that 
doesn't "DCHECK"
 : // in case of collision (check 
thread_collision_warner_unittests.cc)
> nit: it's not the part of your change, but maybe consider updating this des
I'd rather not change that because it's a copy / paste import from Chromium and 
it would make a rebase harder. I tried to avoid non-essential changes in this 
patch.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I038bad901b5235725084bd8671ef4b02ec22c0a7
Gerrit-Change-Number: 9237
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Wed, 07 Feb 2018 19:59:19 +
Gerrit-HasComments: Yes

[kudu-CR] [build] fix on ld's -l:path to file notation on OS X

[Todd Lipcon (Code Review)]

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

Change subject: [build] fix on ld's -l:path_to_file notation on OS X
..

[build] fix on ld's -l:path_to_file notation on OS X

Fixed the sized-deallocation test on OS X.

The -l: works fine with Linux linker, but this notation
is not supported by ld on OS X.  I didn't notice that because I built
on OS X with OUTPUT_QUIET and ERROR_QUIET options already set
for execute_process(), after verifying that it works as need at Linux.

Now, for Kudu builds on OS X 10.12.x and higher the sized deallocation
is enabled, and on OS X 10.11.x it's disabled.

This is a follow-up for c1b9f1088f830235f9d4e6830f59b1a200a65ab6.

Change-Id: I65304c2d7c37abdddb8c60eac0e288b546cfc530
Reviewed-on: http://gerrit.cloudera.org:8080/9242
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M CMakeLists.txt
1 file changed, 3 insertions(+), 1 deletion(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I65304c2d7c37abdddb8c60eac0e288b546cfc530
Gerrit-Change-Number: 9242
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] docs: improvements to transaction semantics

[Hao Hao (Code Review)]

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

Change subject: docs: improvements to transaction semantics
..


Patch Set 1:

> Which version of Kudu does this apply?

The latest release version, 1.6.0.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I23a2751923a4214f52e6cdb233f7e3aeee207da2
Gerrit-Change-Number: 9235
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 07 Feb 2018 19:37:45 +
Gerrit-HasComments: No

[kudu-CR] [build] fix on ld's -l:path to file notation on OS X

[Alexey Serbin (Code Review)]

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


Change subject: [build] fix on ld's -l:path_to_file notation on OS X
..

[build] fix on ld's -l:path_to_file notation on OS X

Fixed the sized-deallocation test on OS X.

The -l: works fine with Linux linker, but this notation
is not supported by ld on OS X.  I didn't notice that because I built
on OS X with OUTPUT_QUIET and ERROR_QUIET options already set
for execute_process(), after verifying that it works as need at Linux.

Now, for Kudu builds on OS X 10.12.x and higher the sized deallocation
is enabled, and on OS X 10.11.x it's disabled.

This is a follow-up for c1b9f1088f830235f9d4e6830f59b1a200a65ab6.

Change-Id: I65304c2d7c37abdddb8c60eac0e288b546cfc530
---
M CMakeLists.txt
1 file changed, 3 insertions(+), 1 deletion(-)



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

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

[kudu-CR] Add back KuduColumnSchema DataTypeToString

[Grant Henke (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add back KuduColumnSchema DataTypeToString
..

Add back KuduColumnSchema DataTypeToString

KuduColumnSchema’s DataTypeToString method
was accidentally removed in KUDU-721.

This method/functionality should be added back
to prevent a compatibility break. However, to
work correctly with the new DECIMAL types
another parameter is required. To handle this
the old method was added back and deprecated
and a new method was added.

Change-Id: I2bf6879f5ab623366a962c6a3d51efc49978a53c
---
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
2 files changed, 25 insertions(+), 1 deletion(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2bf6879f5ab623366a962c6a3d51efc49978a53c
Gerrit-Change-Number: 9240
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Add back KuduColumnSchema DataTypeToString

[Grant Henke (Code Review)]

Grant Henke has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9240


Change subject: Add back KuduColumnSchema DataTypeToString
..

Add back KuduColumnSchema DataTypeToString

KuduColumnSchema’s DataTypeToString method
was accidentally removed in KUDU-721.

This method/functionality should be added back
to prevent a compatibility break. However, to
work correctly with the new DECIMAL types
another parameter is required. To handle this
the old method was added back and deprecated
and a new method was added.

Change-Id: I2bf6879f5ab623366a962c6a3d51efc49978a53c
---
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
2 files changed, 24 insertions(+), 1 deletion(-)



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

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

[kudu-CR] [docs] MacPorts specific build instructions

[Will Berkeley (Code Review)]

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

Change subject: [docs] MacPorts specific build instructions
..


Patch Set 1: Code-Review+1

(1 comment)

I didn't test it but it looks right.

One small worry I have about the patch is that it mostly duplicates the 
build-Kudu-on-macOS script, so two copies need to be maintained. I think this 
is OK since the script shouldn't change very much and the two copies are right 
next to each other.

http://gerrit.cloudera.org:8080/#/c/9236/1/docs/installation.adoc
File docs/installation.adoc:

http://gerrit.cloudera.org:8080/#/c/9236/1/docs/installation.adoc@545
PS1, Line 545: $ brew tap homebrew/dupes
While you're here, I think this is outdated:

 $ brew tap homebrew/dupes
 Error: homebrew/dupes was deprecated. This tap is now empty as all its 
formulae were migrated.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic103c1d883cd94c5e21b291e81005e14324a813f
Gerrit-Change-Number: 9236
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Wed, 07 Feb 2018 16:55:17 +
Gerrit-HasComments: Yes

[kudu-CR] docs: improvements to NTP troubleshooting

[Will Berkeley (Code Review)]

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

Change subject: docs: improvements to NTP troubleshooting
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9234/1/docs/troubleshooting.adoc
File docs/troubleshooting.adoc:

http://gerrit.cloudera.org:8080/#/c/9234/1/docs/troubleshooting.adoc@137
PS1, Line 137: healthhy
Extra h.


http://gerrit.cloudera.org:8080/#/c/9234/1/docs/troubleshooting.adoc@156
PS1, Line 156: informmation
Extra m.


http://gerrit.cloudera.org:8080/#/c/9234/1/docs/troubleshooting.adoc@245
PS1, Line 245: `ERROR`
nit: `ERROR` log



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I07b6871b91ed4ee08992d2fcd093f1054c7d61b8
Gerrit-Change-Number: 9234
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Wed, 07 Feb 2018 16:41:33 +
Gerrit-HasComments: Yes