[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics

[Todd Lipcon (Code Review)]

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

Change subject: KUDU-2031: Add metrics per connection to the reactor metrics
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:

http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h@398
PS3, Line 398: kbps
> Is this bit per second or byte per second?  Please don't use abbreviations
Can you talk a bit more about the use case for this? Do you think something 
like exponential weighted moving average could get you something equally as 
useful with significantly less memory?


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

http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc@662
PS3, Line 662: // Do the transfer and time it.
 : Stopwatch send_timer;
 : send_timer.start();
 : Status status = transfer->SendBuffer(*socket_);
 : send_timer.stop();
> Yeah I think there's a danger that this is just timing how long it takes to
agreed grabbing the time at the start of each transfer and then the time when 
it finished (which may be a diferent iteration of the reactor loop) will more 
accurately capture this.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465
Gerrit-Change-Number: 9343
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 17 Feb 2018 02:22:45 +
Gerrit-HasComments: Yes

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

[Hao Hao (Code Review)]

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

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


Patch Set 9:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8823/8/src/kudu/client/client-internal.h
File src/kudu/client/client-internal.h:

http://gerrit.cloudera.org:8080/#/c/8823/8/src/kudu/client/client-internal.h@197
PS8, Line 197:   // Returns the max observed timestamp from servers that only 
for all
> Please add docs to these, since it's not obvious anymore, e.g. what's the d
Done


http://gerrit.cloudera.org:8080/#/c/8823/8/src/kudu/client/client-internal.h@278
PS8, Line 278:
> As implemented here, isn't there interactions between two concurrently opened 
> RYW scanners?
Right, I think the idea of using a client property is to track the current/max 
observed timestamp across all opened RYW scanners. Whenever there is a new 
scanner open, always use the max of all to ensure read-your-reads.

I am not sure how can we achieve this if we use a scanner property/field.


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

http://gerrit.cloudera.org:8080/#/c/8823/8/src/kudu/client/client.h@1740
PS8, Line 1740: /// When @c READ_YOUR_WRITES is specified, the server will 
pick a timestamp to use
> We should consider 'dumbing down' these docs considerably.  For instance, t
Good point, will add her as a reviewer.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34214245a78aed172a28fbdb395ff5bccd0fc0e1
Gerrit-Change-Number: 8823
Gerrit-PatchSet: 9
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 17 Feb 2018 00:31:29 +
Gerrit-HasComments: Yes

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

[Hao Hao (Code Review)]

Hello Dan Burkert, David Ribeiro Alves, Kudu Jenkins,

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

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

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

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

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

Change-Id: I34214245a78aed172a28fbdb395ff5bccd0fc0e1
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-test.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/integration-tests/consistency-itest.cc
M src/kudu/integration-tests/delete_table-itest.cc
10 files changed, 304 insertions(+), 65 deletions(-)


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

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

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

[Hao Hao (Code Review)]

Hello Tidy Bot, 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 (#16).

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 if possible. 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-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
6 files changed, 321 insertions(+), 98 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/16
--
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: 16
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-Reviewer: Tidy Bot

[kudu-CR] KUDU-2297 (part 3): refactor process-wide stack collection out of /stacks

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins,

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

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

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

Change subject: KUDU-2297 (part 3): refactor process-wide stack collection out 
of /stacks
..

KUDU-2297 (part 3): refactor process-wide stack collection out of /stacks

Previously a bunch of logic to collect all the stacks from the process
was in the /stacks path handler. This logic is relatively generic and
shouldn't be intermingled with the formatting code. In particular I'd
like to use it in the diagnostics log, where a different output format
is desirable.

Change-Id: Ibb7c6edd31254f3d7e0cbef1eaf575bde3570df6
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/server/diagnostics_log.cc
M src/kudu/server/diagnostics_log.h
M src/kudu/util/debug-util-test.cc
M src/kudu/util/debug-util.cc
M src/kudu/util/debug-util.h
6 files changed, 184 insertions(+), 112 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibb7c6edd31254f3d7e0cbef1eaf575bde3570df6
Gerrit-Change-Number: 9329
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2291 (part 5): allow collecting stack traces asynchronously

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins,

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

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

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

Change subject: KUDU-2291 (part 5): allow collecting stack traces asynchronously
..

KUDU-2291 (part 5): allow collecting stack traces asynchronously

This removes the global variable previously used to communicate between
a stack trace collector and the stack trace target. The global variable
had the downside of allowing only a single stack trace operation in
flight at a time, which means that for use cases where we want to
capture the trace of a bunch of threads, we needed to do so serially.

For use cases like process-wide traces this could make debugging a bit
tricky. For example, because of the skew in time between collecting
the stack of thread A and the stack of thread B, it's possible that
the traces could show them holding the same lock, which we know to be
impossible. Getting the collection time as close as possible to
"simultaneous" can reduce the possibility of these kind of
strange-looking results.

The actual mechanism to do this is slightly tricky. Rather than
duplicate the full description here, check the comments in the
implementation. In particular, StackTraceCollector::RevokeSigData()
explains the complexity of not knowing whether our signal will ever be
delivered.

A new unit test shows the pattern, and this also updates the stack
servlet to send the signals to all of the threads as close as possible
to "the same time".

Change-Id: Ie4d7789bb1272db033df4685f46eaf8483ec7be1
---
M src/kudu/server/default_path_handlers.cc
M src/kudu/util/debug-util-test.cc
M src/kudu/util/debug-util.cc
M src/kudu/util/debug-util.h
4 files changed, 309 insertions(+), 109 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie4d7789bb1272db033df4685f46eaf8483ec7be1
Gerrit-Change-Number: 9318
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2275. Upgrade libunwind to 1.3-rc1

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Kudu Jenkins,

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

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

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

Change subject: KUDU-2275. Upgrade libunwind to 1.3-rc1
..

KUDU-2275. Upgrade libunwind to 1.3-rc1

Per KUDU-2275, the version of libunwind that we were previously using
can occasionally crash during stack unwinding if it attempts to access a
page which is mprotected to be non-readable. This could be due to
incorrect interpretation of dwarf unwinding info or some other bug.

As we are trying to collect stacks more aggressively now, it's important
to upgrade. This new version uses a more robust mechanism for checking
whether a page is valid to access before accessing it during unwinding.

This also includes a patch from the libunwind git repo which fixes a
potential issue with libunwind in ASAN builds. I didn't run into this
issue yet myself, but seems like a straightforward and necessary fix.

Change-Id: I3f942a0d566e1b1e5ebe48f09362bf8aa3e1e33e
---
M src/kudu/util/file_cache-test.cc
M thirdparty/download-thirdparty.sh
A 
thirdparty/patches/libunwind-Use-syscall-directly-in-write_validate-to-avoid-ASAN.patch
M thirdparty/vars.sh
4 files changed, 61 insertions(+), 4 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3f942a0d566e1b1e5ebe48f09362bf8aa3e1e33e
Gerrit-Change-Number: 9335
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2297 (part 2). Convert diagnostics log to a format closer to glog

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins,

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

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

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

Change subject: KUDU-2297 (part 2). Convert diagnostics log to a format closer 
to glog
..

KUDU-2297 (part 2). Convert diagnostics log to a format closer to glog

Previously the metrics/diagnostics log used a custom log format designed
to be readable by scripts. The only timestamps were unix microtimes,
which are convenient for computers but not for people. In order to make
the log more easily greppable, this patch converts it over to use a
glog-compatible prefix on each log line.

The aim is that, if an admin sees some issues at 10/23 15:03 they can simply
grep the diagnostics log for '1021 15:0[0-6]' rather than having to go
figure out the appropriate range of numeric timestamps.

This patch also updates the docs accordingly.

Change-Id: I10d93c84f8c738c26850ae3d1ae6afbb5519c7ad
---
M docs/administration.adoc
M src/kudu/server/diagnostics_log.cc
M src/kudu/util/logging.cc
M src/kudu/util/logging.h
M src/kudu/util/trace.cc
5 files changed, 45 insertions(+), 28 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I10d93c84f8c738c26850ae3d1ae6afbb5519c7ad
Gerrit-Change-Number: 9327
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2297 (part 1): refactor metrics log out of ServerBase

[Todd Lipcon (Code Review)]

Hello Will Berkeley, Tidy Bot, Mike Percy, Kudu Jenkins,

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

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

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

Change subject: KUDU-2297 (part 1): refactor metrics log out of ServerBase
..

KUDU-2297 (part 1): refactor metrics log out of ServerBase

This is preparing for putting more information into the metrics log
instead of just metrics snapshots. The logging code will get more
complex as it gains features, so this makes a new class for it.

This is slightly incompatible since I also changed the name on disk to
'diagnostics' instead of 'metrics' and updated the documentation, but I
am not aware of people using this in the wild. So long as we
release-note it, I think it's reasonable to expect any sophisticated
operators to adjust their scripting accordingly.

Change-Id: I1f7bc7335b1f4c2a5c62fc5fe672289d5027a10d
---
M docs/administration.adoc
M src/kudu/server/CMakeLists.txt
A src/kudu/server/diagnostics_log.cc
A src/kudu/server/diagnostics_log.h
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
6 files changed, 237 insertions(+), 73 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1f7bc7335b1f4c2a5c62fc5fe672289d5027a10d
Gerrit-Change-Number: 9326
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 

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

[David Ribeiro Alves (Code Review)]

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

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


Patch Set 16: Code-Review+2


--
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: 16
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-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 17 Feb 2018 00:36:50 +
Gerrit-HasComments: No

[kudu-CR] [kudu-jepsen] run kudu CLI tool under 'kudu' user

[Hao Hao (Code Review)]

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

Change subject: [kudu-jepsen] run kudu CLI tool under 'kudu' user
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e556e901308871d0bbe6091be41a7a365421222
Gerrit-Change-Number: 9356
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Sat, 17 Feb 2018 00:37:42 +
Gerrit-HasComments: No

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

[Alexey Serbin (Code Review)]

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

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


Patch Set 16:

(6 comments)

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

http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_server-test.cc@a1726
PS16, Line 1726:
What happened to this test?  Maybe I'm missing something, but it seems this 
test should stay relevant regardless of the new scan mode.


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

http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@157
PS16, Line 157: TimeStamp
nit: in the code around, we have Timestamp as a type; maybe, name it 
consistently with that so it's ValidateTimestamp(...) ?


http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@161
PS16, Line 161: PickAndVerifyTimeStamp
nit: ditto, PickAndVerifyTimestamp(...) ?


http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.h@162
PS16, Line 162: tablet::TabletReplica*
Could it be const tablet::TabletReplica& ?

Or, maybe, Tablet* ?


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

http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@1757
PS16, Line 1757:   case READ_YOUR_WRITES:
nit: per coding standard, add // fallthrough


http://gerrit.cloudera.org:8080/#/c/8804/16/src/kudu/tserver/tablet_service.cc@2042
PS16, Line 2042: case READ_AT_SNAPSHOT:
nit: per coding standard, add // fallthrough



--
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: 16
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-Reviewer: Tidy Bot
Gerrit-Comment-Date: Sat, 17 Feb 2018 01:44:55 +
Gerrit-HasComments: Yes

[kudu-CR] [kudu-jepsen] run kudu CLI tool under 'kudu' user

[Alexey Serbin (Code Review)]

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

Change subject: [kudu-jepsen] run kudu CLI tool under 'kudu' user
..

[kudu-jepsen] run kudu CLI tool under 'kudu' user

Since cd41990 was committed, the 'kudu cluster ksck' CLI tool command
started returning failure exit code if running not under the service
superuser account (i.e. not under 'kudu' user) because fetching Raft
consensus information on a tablet requires service superuser-level
privileges.

To fix that, at least the two alternatives are available:
  * Run the CLI tool under the service superuser account ('kudu' user).
  * Run the 'kudu cluster ksck' sub-command with the
--consensus=false option, so it would not try to fetch consensus
information on existing tablets.

The former alternative makes more sense because the list of privileges
required to call server methods is evolving.  Also, in future we may
need to do other queries using the kudu CLI tool in the context of
Jepsen tests for Kudu.

Change-Id: I7e556e901308871d0bbe6091be41a7a365421222
Reviewed-on: http://gerrit.cloudera.org:8080/9356
Tested-by: Kudu Jenkins
Reviewed-by: Hao Hao 
---
M java/kudu-jepsen/src/main/clojure/jepsen/kudu/util.clj
1 file changed, 4 insertions(+), 3 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Hao Hao: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7e556e901308871d0bbe6091be41a7a365421222
Gerrit-Change-Number: 9356
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics

[Alexey Serbin (Code Review)]

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

Change subject: KUDU-2031: Add metrics per connection to the reactor metrics
..


Patch Set 3:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:

http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h@241
PS3, Line 241:   double rough_kbps() const {
why not to move the implementation into the connection.cc file?


http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h@245
PS3, Line 245: for (int i = 0; i < rolling_window_size; ++i) {
 :   rough_kbps += rolling_kbps_[i];
 : }
syntactic sugar nit: I think it might be just

  rough_kbps = std::accumulate(rolling_kbps_.begin(), rolling_kbps_.end(), 0.0);


http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h@398
PS3, Line 398: kbps
Is this bit per second or byte per second?  Please don't use abbreviations in 
the comment to keep it clear.


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

http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc@82
PS3, Line 82: 1000
Does it make sense to control the size of the rolling window by a gflag?  That 
way it would be possible to update it in run time using 
GenericServiceImpl::SetFlag(), at least, in Kudu.


http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:

http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/rpc-test.cc@471
PS3, Line 471: CHECK_OK
why not just ASSERT_OK?


http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/rpc-test.cc@472
PS3, Line 472: CHECK_OK
ditto


http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/rpc-test.cc@502
PS3, Line 502: p.AsyncRequest(GenericCalculatorService::kAddMethodName, 
add_req, _resp,
 : controllers.back().get(), 
boost::bind(::CountDown, boost::ref(latch)));
we tend to use std::bind instead of boost::bind in newer tests, if possible



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465
Gerrit-Change-Number: 9343
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 17 Feb 2018 01:04:36 +
Gerrit-HasComments: Yes

[kudu-CR](branch-1.5.x) KUDU-2238. DMS not flush under memory pressure.

[Will Berkeley (Code Review)]

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

Change subject: KUDU-2238. DMS not flush under memory pressure.
..


Patch Set 2:

> Build Failed
 >
 > http://jenkins.kudu.apache.org/job/kudu-gerrit/12030/ : FAILURE

Seems like the python tests are failing because the build slave is too old?

"your setuptools is too old (<12)
setuptools_scm functionality is degraded"

Then after that the build outputs for debug v release are very different, and 
it doesn;t seem the python tests even run in debug though there's no error 
message beyond an assertion the tests failed.


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

Gerrit-Project: kudu
Gerrit-Branch: branch-1.5.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c04d76aa0e3888352dc56eeb493a5437ef47e42
Gerrit-Change-Number: 9322
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Reviewer: Will Berkeley 
Gerrit-Reviewer: zhen.zhang 
Gerrit-Comment-Date: Fri, 16 Feb 2018 16:07:36 +
Gerrit-HasComments: No

[kudu-CR] [docs] Bump release notes in preparation for 1.7 release

[Grant Henke (Code Review)]

Grant Henke has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9342 )

Change subject: [docs] Bump release notes in preparation for 1.7 release
..

[docs] Bump release notes in preparation for 1.7 release

Change-Id: I504ec46082e7303a00bb3f3f92d6305fa9c20148
Reviewed-on: http://gerrit.cloudera.org:8080/9342
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy 
---
M docs/prior_release_notes.adoc
M docs/release_notes.adoc
2 files changed, 204 insertions(+), 138 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Mike Percy: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I504ec46082e7303a00bb3f3f92d6305fa9c20148
Gerrit-Change-Number: 9342
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 

[kudu-CR] [spark] enable scan locality by default

[Dan Burkert (Code Review)]

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

Change subject: [spark] enable scan locality by default
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifc94e9ce52877a88f96a06599a8657da929b6126
Gerrit-Change-Number: 9163
Gerrit-PatchSet: 1
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 16 Feb 2018 18:59:43 +
Gerrit-HasComments: No

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

[Dan Burkert (Code Review)]

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

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


Patch Set 14: Code-Review+1


--
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: 14
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-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 16 Feb 2018 18:58:54 +
Gerrit-HasComments: No

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

[Dan Burkert (Code Review)]

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

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


Patch Set 8:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8823/8/src/kudu/client/client-internal.h
File src/kudu/client/client-internal.h:

http://gerrit.cloudera.org:8080/#/c/8823/8/src/kudu/client/client-internal.h@197
PS8, Line 197:   uint64_t GetMaxObservedTimestamp() const;
Please add docs to these, since it's not obvious anymore, e.g. what's the 
difference between GetLatestObserved and GetMaxObserverd


http://gerrit.cloudera.org:8080/#/c/8823/8/src/kudu/client/client-internal.h@278
PS8, Line 278:   AtomicInt max_observed_timestamp_;
I didn't expect an additional field on the client here, instead I though the 
scanner would get a new field 'propagated_timestamp' that would be used in RYW 
scans.  Instead of using the client's latest_observed_timestamp() as the 
propagated timestamp it would use that field.

As implemented here, isn't there interactions between two concurrently opened 
RYW scanners?


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

http://gerrit.cloudera.org:8080/#/c/8823/8/src/kudu/client/client.h@1740
PS8, Line 1740: /// When @c READ_YOUR_WRITES is specified, the server will 
pick a snapshot timestamp
We should consider 'dumbing down' these docs considerably.  For instance, the 
client really shouldn't need to know anything about timestamps or in-flight 
transactions to use or pick this scan mode.  I think it's fine to get into this 
level of specificity on the READ_YOUR_WRITES docs in the .proto, since that's 
internal, however these docs have a different audience.  I'd try and re-focus 
this doc on the characteristics of the scan from the perspective of the client 
application.  I'd also consider asking Alexandra to do do a pass on this.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34214245a78aed172a28fbdb395ff5bccd0fc0e1
Gerrit-Change-Number: 8823
Gerrit-PatchSet: 8
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 16 Feb 2018 19:36:02 +
Gerrit-HasComments: Yes

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

[David Ribeiro Alves (Code Review)]

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

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


Patch Set 14:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.h@156
PS14, Line 156:   // as such timestamp is invalid.
nit: "such a"


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

http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc@2046
PS14, Line 2046: return Status::NotSupported("Unsupported snapshot scan mode 
specified");
nit: I think this can FATAL out. it's us (not the user) that call this and we 
should only call it for "snapshotty" scans


http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc@2166
PS14, Line 2166: RETURN_NOT_OK(ValidateTimeStamp(tmp_snap_timestamp));
What is this really checking. We need to validate the timestamp for 
READ_AT_SNAPSHOT as the user can send a timestamp that is arbitrarily in the 
future, but here it can only be either "clean_timestamp" (which is valid) or 
propagated timestamp, which is necessarily valid since we've udpated the clock 
with it in line 2139.
In other words do you think that there is a test case you can write that would 
fail this check?



--
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: 14
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-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 16 Feb 2018 20:08:57 +
Gerrit-HasComments: Yes

[kudu-CR] java: improve error messages when tokens are not used

[Dan Burkert (Code Review)]

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

Change subject: java: improve error messages when tokens are not used
..


Patch Set 1:

Would be great to get this in for 1.7.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f5160a79e973c411c16a784dc105444f5dd2a6f
Gerrit-Change-Number: 9070
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 16 Feb 2018 19:01:21 +
Gerrit-HasComments: No

[kudu-CR] KUDU-2295 fix nullptr dereference in Tablet

[Alexey Serbin (Code Review)]

Alexey Serbin has abandoned this change. ( http://gerrit.cloudera.org:8080/9350 
)

Change subject: KUDU-2295 fix nullptr dereference in Tablet
..


Abandoned

Ah, I take it back -- the scoped_refptr consumes RefCountedThreadSafe, and 
those ops seems to be safe.  I confused that with shared_ptr.
--
To view, visit http://gerrit.cloudera.org:8080/9350
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Id133f33e45e5281200fc941faa72480caf34de78
Gerrit-Change-Number: 9350
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2031: Add metrics per connection to the reactor metrics

[Dan Burkert (Code Review)]

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

Change subject: KUDU-2031: Add metrics per connection to the reactor metrics
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h
File src/kudu/rpc/connection.h:

http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.h@25
PS3, Line 25: #include 
prefer using 


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

http://gerrit.cloudera.org:8080/#/c/9343/3/src/kudu/rpc/connection.cc@662
PS3, Line 662: // Do the transfer and time it.
 : Stopwatch send_timer;
 : send_timer.start();
 : Status status = transfer->SendBuffer(*socket_);
 : send_timer.stop();
> Not sure how accurate this will be fore inferring throughput given this is
Yeah I think there's a danger that this is just timing how long it takes to 
copy the bytes to the nic or tcp send buffer.  It may be more accurate to start 
the timer beginning of the transfer and stop it after it's completely sent, 
however that's still going to be inflated for small messages.

I looked a little bit into this, and it seems like we may be able to get the 
info we want from getsockopt/TCP_INFO, however you will need to keep this 
rolling-window setup since it's just a point in time snapshot.  
https://linuxgazette.net/136/pfeiffer.html has a good writeup.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae1a5fe0066adf644a9cac41ad6696e1bbf00465
Gerrit-Change-Number: 9343
Gerrit-PatchSet: 3
Gerrit-Owner: Sailesh Mukil 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 16 Feb 2018 18:14:44 +
Gerrit-HasComments: Yes

[kudu-CR] [rpc] fix typo in Messenger::metric entity()

[Todd Lipcon (Code Review)]

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

Change subject: [rpc] fix typo in Messenger::metric_entity()
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic976a5038e982d92318ca5441866fbd05858b92a
Gerrit-Change-Number: 9351
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 16 Feb 2018 21:27:50 +
Gerrit-HasComments: No

[kudu-CR] [rpc] fix typo in Messenger::metric entity()

[Todd Lipcon (Code Review)]

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

Change subject: [rpc] fix typo in Messenger::metric_entity()
..

[rpc] fix typo in Messenger::metric_entity()

Change-Id: Ic976a5038e982d92318ca5441866fbd05858b92a
Reviewed-on: http://gerrit.cloudera.org:8080/9351
Tested-by: Alexey Serbin 
Reviewed-by: Todd Lipcon 
---
M src/kudu/rpc/messenger.h
1 file changed, 1 insertion(+), 1 deletion(-)

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

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

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

[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 14:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc@2166
PS14, Line 2166: RETURN_NOT_OK(ValidateTimeStamp(tmp_snap_timestamp));
> What is this really checking. We need to validate the timestamp for READ_AT
Right, think a bit more, I do not see a case where this validation could fail 
(which essentially mean: propagation TS + 1 > propagation TS + 
FLAGS_max_clock_sync_error_usec)

So it may not worth checking here. I will remove.



--
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: 14
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-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 16 Feb 2018 22:44:40 +
Gerrit-HasComments: Yes

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

[David Ribeiro Alves (Code Review)]

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

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


Patch Set 12:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2114
PS12, Line 2114:   // Note: if 'max_allowed_ts' is not obtained from 
clock_->GetGlobalLatest() it's guaranteed
   :   // to be higher than 'tmp_snap_timestamp'.
> Good question. TBH, this is refactored from previous code, so I am not sure
Good question/point. I wrote that and it was cryptic to me too. I now realize 
that I'm just referring to the fact that if we don't set a timestamp at all, it 
get's set to MAX_LONG - 1 (kInvalidTimestamp). The point is to put reader at 
ease that even if we don't get a global latest ts from the clock, it's still 
safe to compare the snap timestamp below.

Thanks for fixing this Hao



--
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: 12
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-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 16 Feb 2018 22:50:13 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2305: Fix local variable usage to handle 2GB messages

[Joe McDonnell (Code Review)]

Joe McDonnell has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9355


Change subject: KUDU-2305: Fix local variable usage to handle 2GB messages
..

KUDU-2305: Fix local variable usage to handle 2GB messages

The maximum value for rpc_max_message_size (an int32_t) is
INT_MAX. However, when using this maximum value, certain
local variables in SerializeMessage() and InboundTransfer
can overflow if the message size approaches INT_MAX.

This changes certain variables to handle messages that
approach INT_MAX. Specifically:
 - Local variables in SerializeMessage() become uint32_t
   rather than int.
 - The total message size prefixed to the message is now
   treated as a uint32_t everywhere. This impacts
   InboundTransfer::total_length_/cur_offset_ and
   a local variable in ParseMessage().

These changes mean that a sender can serialize a message
that is slightly larger than INT_MAX due to the added
header size, but the receiver will reject all messages
exceeding rpc_max_message_size (maximum INT_MAX).

For testing, this modifies rpc-test's TestRpcSidecarLimits
to send a message of size INT_MAX rather than
rpc_max_message_size+1.

Change-Id: I8e468099b16f7eb55de71b753acae8f57d18287c
---
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
4 files changed, 15 insertions(+), 8 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8e468099b16f7eb55de71b753acae8f57d18287c
Gerrit-Change-Number: 9355
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 

[kudu-CR] KUDU-2305: Fix local variable usage to handle 2GB messages

[Todd Lipcon (Code Review)]

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

Change subject: KUDU-2305: Fix local variable usage to handle 2GB messages
..


Patch Set 1:

(4 comments)

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

http://gerrit.cloudera.org:8080/#/c/9355/1//COMMIT_MSG@16
PS1, Line 16:  - Local variables in SerializeMessage() become uint32_t
:rather than int.
what about just using int64 for the variables? we try not to use unsigneds when 
possible


http://gerrit.cloudera.org:8080/#/c/9355/1//COMMIT_MSG@26
PS1, Line 26: exceeding rpc_max_message_size (maximum INT_MAX).
should we add validation to the flag to ensure that it isn't set to any value 
larger than INT_MAX-4 or whatever the true maximum is?


http://gerrit.cloudera.org:8080/#/c/9355/1/src/kudu/rpc/serialization.cc
File src/kudu/rpc/serialization.cc:

http://gerrit.cloudera.org:8080/#/c/9355/1/src/kudu/rpc/serialization.cc@59
PS1, Line 59:   // KUDU-2305: Use unsigned 4-byte integers to avoid overflowing 
when additional_size
this sort of makes it sound like you're pointing out an existing bug instead of 
documenting that the fix is related to this JIRA. I think better to not 
reference the JIRA and if someone is confused they can find the JIRA via git 
blame


http://gerrit.cloudera.org:8080/#/c/9355/1/src/kudu/rpc/serialization.cc@120
PS1, Line 120:   DCHECK_EQ(total_len + kMsgLengthPrefixLength, buf.size())
maybe safer here to subtract from buf.size rather than add to total_len, so it 
can't overflow even if the caller sends UINT32_MAX



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8e468099b16f7eb55de71b753acae8f57d18287c
Gerrit-Change-Number: 9355
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 16 Feb 2018 23:27:09 +
Gerrit-HasComments: Yes

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

[Hao Hao (Code Review)]

Hello Tidy Bot, 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 (#15).

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 if possible. 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-base.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/tablet_service.h
6 files changed, 321 insertions(+), 98 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8804/15
--
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: 15
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-Reviewer: Tidy Bot

[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 14:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.h@156
PS14, Line 156:   // as such timestamp is invalid.
> nit: "such a"
Done


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

http://gerrit.cloudera.org:8080/#/c/8804/12/src/kudu/tserver/tablet_service.cc@2114
PS12, Line 2114:   }
   :
> Good question/point. I wrote that and it was cryptic to me too. I now reali
I see, thanks David for the explanation.


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

http://gerrit.cloudera.org:8080/#/c/8804/14/src/kudu/tserver/tablet_service.cc@2046
PS14, Line 2046: return Status::NotSupported("Unsupported snapshot scan mode 
specified");
> nit: I think this can FATAL out. it's us (not the user) that call this and
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: 14
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-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 16 Feb 2018 23:26:23 +
Gerrit-HasComments: Yes

[kudu-CR] [spark] enable scan locality by default

[Hao Hao (Code Review)]

Hao Hao has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/9163 )

Change subject: [spark] enable scan locality by default
..

[spark] enable scan locality by default

Commit 3abca98c5 introduced support to take advantage of scan locality
in Spark integration, however this feature is not enabled by default.
This patch defaults scan locality to use closest replica. And if the
client configures to not to use the default, then only list leader
replica as the perferred location to schedule a task.

Change-Id: Ifc94e9ce52877a88f96a06599a8657da929b6126
Reviewed-on: http://gerrit.cloudera.org:8080/9163
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
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/KuduRDD.scala
2 files changed, 12 insertions(+), 4 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Dan Burkert: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ifc94e9ce52877a88f96a06599a8657da929b6126
Gerrit-Change-Number: 9163
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [kudu-jepsen] run kudu CLI tool under 'kudu' user

[Alexey Serbin (Code Review)]

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


Change subject: [kudu-jepsen] run kudu CLI tool under 'kudu' user
..

[kudu-jepsen] run kudu CLI tool under 'kudu' user

Since cd41990 was committed, the 'kudu cluster ksck' CLI tool command
started returning failure exit code if running not under the service
superuser account (i.e. not under 'kudu' user) because fetching Raft
consensus information on a tablet requires service superuser-level
privileges.

To fix that, at least the two alternatives are available:
  * Run the CLI tool under the service superuser account ('kudu' user).
  * Run the 'kudu cluster ksck' sub-command with the
--consensus=false option, so it would not try to fetch consensus
information on existing tablets.

The former alternative makes more sense because the list of privileges
required to call server methods is evolving.  Also, in future we may
need to do other queries using the kudu CLI tool in the context of
Jepsen tests for Kudu.

Change-Id: I7e556e901308871d0bbe6091be41a7a365421222
---
M java/kudu-jepsen/src/main/clojure/jepsen/kudu/util.clj
1 file changed, 4 insertions(+), 3 deletions(-)



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

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