[kudu-CR] tablet: change default bloom filter FP rate to 0.01%

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: tablet: change default bloom filter FP rate to 0.01%
..


Patch Set 2: Code-Review+2

thanks for adding the additional info

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99bdd6298349a5be5f1fc3a666fe04305699e293
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] tablet: change default bloom filter FP rate to 0.01%

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: tablet: change default bloom filter FP rate to 0.01%
..


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3517/2//COMMIT_MSG
Commit Message:

PS2, Line 25: 
https://gist.github.com/toddlipcon/1ab9b36b7fbae10b635d3a905e1fe55a
interesting how the changed graph seems to have a second life towards the end.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99bdd6298349a5be5f1fc3a666fe04305699e293
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] tablet: change default bloom filter FP rate to 0.01%

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: tablet: change default bloom filter FP rate to 0.01%
..


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/2079/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99bdd6298349a5be5f1fc3a666fe04305699e293
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] tablet: change default bloom filter FP rate to 0.01%

[Todd Lipcon (Code Review)]

Hello Jean-Daniel Cryans, Kudu Jenkins,

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

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

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

Change subject: tablet: change default bloom filter FP rate to 0.01%
..

tablet: change default bloom filter FP rate to 0.01%

The old default, 1%, was high enough that in a uniform random write workload,
we ended up needing to read in most of the key blocks even with bloom filters
enabled. On a 5 node cluster, after inserting a few billion rows, the write
throughput dropped dramatically as every batch of writes was seeking and
reading keys off disk.

In testing on the same cluster, changing the FP rate to 0.01% improved the
throughput dramatically (>2x) by reducing the random reads coming off disk. The
cost is a 2x increase in bloom filter size (20 bits per key vs 10) but
20 bits is still a small percentage compared to typical row key sizes
in target applications.

Of course if an application has no random write characteristics and really
cares about disk space, this can always be flipped back.

Screenshots of the inserts/second graph (1hr rolling average) for these tests
are at: https://gist.github.com/toddlipcon/1ab9b36b7fbae10b635d3a905e1fe55a

Change-Id: I99bdd6298349a5be5f1fc3a666fe04305699e293
---
M docs/release_notes.adoc
M src/kudu/tablet/tablet.cc
2 files changed, 10 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I99bdd6298349a5be5f1fc3a666fe04305699e293
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1309: [java client] support tables with non-covering partition-key ranges

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1309: [java client] support tables with non-covering 
partition-key ranges
..


Patch Set 4:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/3388/4/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:

Line 810:   } else {
This will need a rebase + conflict resolution.


Line 1152:   Map.Entry nonCoveredRange = 
getNonCoveredRange(table.getTableId(), key);
Can use tableId here.


http://gerrit.cloudera.org:8080/#/c/3388/4/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:

Line 327:   operation.callback(response);
Does this invoke the user callback eventually? If so, why do it before adding 
the error to the error collector?


PS4, Line 784: private Object tablet = null;
Ugh. Can you add more color on why this approach was necessary? Why can't we 
store the results in two separate fields, one LocatedTablet and one exception?


http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java
File java/kudu-client/src/main/java/org/kududb/client/NonCoveredRangeCache.java:

Line 84:   @Override
> if concurrent updates happen to the entries the iteration is undefined, so 
Really? I thought ConcurrentSkipListMap defines an iteration order in the face 
of concurrent mutations. Javadoc says: "Insertion, removal, update, and access 
operations safely execute concurrently by multiple threads. Iterators are 
weakly consistent, returning elements reflecting the state of the map at some 
point at or since the creation of the iterator. They do not throw 
ConcurrentModificationException, and may proceed concurrently with other 
operations."


Line 89: for (Map.Entry range : 
nonCoveredRanges.entrySet()) {
> If you used a Guava Joiner to convert this list of Strings into a single co
No love for Joiner? Bear in mind size() is more expensive on a 
ConcurrentSkipListMap.


http://gerrit.cloudera.org:8080/#/c/3388/2/java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java
File java/kudu-client/src/test/java/org/kududb/client/TestKuduSession.java:

Line 221:   public void testInsertManualFlushNonCoveredRange() throws Exception 
{
> Replicas aren't important for these particular tests.  Additionally, they c
Right, but I don't think the other tests in this class particularly care about 
replication either (testUpsert() for example). Could you make a pass over all 
of the TestKuduSession tests and switch every test to single-replica, if it 
makes sense? It's more cognitive load as it is now; the reader's eyes are drawn 
to the inconsistency.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id65a1f8a95bb16fc0360a17021a391afd3c9d03f
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] locks: change kudu::shared lock constructor to pass by ref

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: locks: change kudu::shared_lock constructor to pass by ref
..


Patch Set 5: Verified+1

There was a build failure in the RELEASE build:
  
  01:36:24 Linking CXX shared library ../../../lib/libkrpc.so
  01:36:25 CMakeFiles/krpc.dir/constants.cc.o: file not recognized: File 
truncated
  01:36:25 collect2: error: ld returned 1 exit status
  01:36:25 make[2]: *** [lib/libkrpc.so] Error 1
  01:36:25 make[1]: *** [src/kudu/rpc/CMakeFiles/krpc.dir/all] Error 2
  01:36:25 make[1]: *** Waiting for unfinished jobs

That file isn't touched by this patch, all of the other builds passed, and 
subsequent RELEASE builds (building off of this patch) have also passed, so I'm 
overriding Jenkins.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e25a4f519f5cc09f08f8aeda5284eabdadd0c46
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] locks: switch from boost::shared mutex to new read-write mutex

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: locks: switch from boost::shared_mutex to new read-write mutex
..


Patch Set 5: Verified+1

Overriding Jenkins, flaky ksck timeout test.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b4b1df267468dc8c88c3de8e0345c61900576de
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [java-client] refactor AsyncKuduSession

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 3:

(16 comments)

Achievement unlocked: asynchronous programming wizard.

http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:

Line 1410:Arrays.copyOf(partitionKey, 
partitionKey.length + 1), deadline);
So this is how we obtain the very next value following partitionKey, right? 
Surprised it's so simple given how complicated 
EncodedKey::IncrementEncodedKey() is.


http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:

Line 123:* application is {@link #apply}ing operations to one buffer (the 
{@code activeBuffer}), the
Nit: {@link #apply} or {@code apply}? I see both here, not sure which is more 
correct.


Line 131:   private Buffer activeBuffer;
Like the C++ client's batcher abstraction? Nice.


Line 283: return flush();
Why flush() if the session is already closed?


PS3, Line 350:   buffer.callbackFlushNotification();
 :   inactiveBuffers.add(buffer);
 :   flushNotification.getAndSet(new 
Deferred()).callback(null);
And here there's the same sequence of events as in doFlush(), but in a 
different order. Why?


PS3, Line 402:   inactiveBuffers.add(buffer);
 :   // Send buffer available notification.
 :   flushNotification.getAndSet(new 
Deferred()).callback(null);
 :   buffer.callbackFlushNotification();
AFAICT, there's no lock held for this work. The order strikes me as strange: 
we're adding buffer to inactiveBuffers (thus making it globally visible) before 
invoking callbacks, especially the buffer's own callback. Is this safe?


Line 409: Deferred batchResponses = new Deferred<>();
Nit: not really a fan of sometimes using Deferred.fromResult() and other times 
invoking the Deferred constructor directly. Can we be more consistent?


PS3, Line 425: private static final ConvertBatchToListOfResponsesCB INSTANCE =
 : new ConvertBatchToListOfResponsesCB();
Is this for correctness or performance? Seems like the implementation would be 
simpler if we didn't maintain a singleton.


Line 563: doFlush(fullBuffer);
What happens if doFlush() throws but we're executing it because we throw a 
PleaseThrottle or NonRecoverable exception in flush()?


Line 704:   private final class Buffer {
Shouldn't Buffer be declared static? Or did you intend for it to be an inner 
class? I don't see any accesses to the outer class; maybe I missed them though.

Nit: not really clear on why you'd use final here. I don't think there's any 
real danger of someone extending Buffer, so final just adds noise, no?


Line 748:   flushNotification = new Deferred<>();
Nit: use fromResult(null) here to be consistent with the inline field 
constructor.

Better yet, perhaps add a no-arg constructor that initializes via reset()? Then 
it's even more consistent.


Line 756: .add("flusherTask", flusherTask)
This will just be an object address, right? Useful?


http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/client/Batch.java
File java/kudu-client/src/main/java/org/kududb/client/Batch.java:

Line 34: import org.kududb.tserver.Tserver;
Odd that this comes after Tserver.TabletServerErrorPB.


Line 46:   final List operations = new ArrayList<>(1250);
What's magical about 1250?


http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/client/Operation.java
File java/kudu-client/src/main/java/org/kududb/client/Operation.java:

Line 203:   static Tserver.WriteRequestPB.Builder 
createAndFillWriteRequestPB(List operations) {
What motivated the changes here from variadic args to lists? Isn't the former 
(marginally) more performant?


http://gerrit.cloudera.org:8080/#/c/3477/3/java/kudu-client/src/main/java/org/kududb/util/AsyncUtil.java
File java/kudu-client/src/main/java/org/kududb/util/AsyncUtil.java:

Line 22: import java.util.List;
Unused?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] tablet: change default bloom filter FP rate to 0.01%

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: tablet: change default bloom filter FP rate to 0.01%
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3517/1/docs/release_notes.adoc
File docs/release_notes.adoc:

PS1, Line 72: substantially
how substantantially?


PS1, Line 74: incremental
which increase?

Can you provide rough numbers for both?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99bdd6298349a5be5f1fc3a666fe04305699e293
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] Persistent cache support for NVM

[Sarah Jelinek (Code Review)]

Sarah Jelinek has posted comments on this change.

Change subject: Persistent cache support for NVM
..


Patch Set 16:

(1 comment)

Pushed all code review changes.

http://gerrit.cloudera.org:8080/#/c/2593/15/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:

Line 54: TAG_FLAG(nvm_cache_path, experimental);
> The reason for this is that when operating in persistent mode the user has 
I died modify this to be nvm_cache_pool to indicate that when persistent memory 
is in use a pool with a specified name is created. Otherwise it's just a 
directory where a pool is created.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id72570ead662670bf42175756a18ae08d7cd0a07
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sarah Jelinek 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sarah Jelinek 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Persistent cache support for NVM

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Persistent cache support for NVM
..


Patch Set 16:

Build Started http://104.196.14.100/job/kudu-gerrit/2078/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id72570ead662670bf42175756a18ae08d7cd0a07
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sarah Jelinek 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sarah Jelinek 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Persistent cache support for NVM

[Sarah Jelinek (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Persistent cache support for NVM
..

Persistent cache support for NVM

Change-Id: Id72570ead662670bf42175756a18ae08d7cd0a07
---
M build-support/run-test.sh
M src/kudu/cfile/block_cache.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
A src/kudu/util/nvm_cache-test.cc
M src/kudu/util/nvm_cache.cc
10 files changed, 713 insertions(+), 201 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id72570ead662670bf42175756a18ae08d7cd0a07
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Sarah Jelinek 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sarah Jelinek 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] tablet: change default bloom filter FP rate to 0.01%

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: tablet: change default bloom filter FP rate to 0.01%
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I99bdd6298349a5be5f1fc3a666fe04305699e293
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] tablet: change default bloom filter FP rate to 0.01%

[Todd Lipcon (Code Review)]

Hello Jean-Daniel Cryans,

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

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

to review the following change.

Change subject: tablet: change default bloom filter FP rate to 0.01%
..

tablet: change default bloom filter FP rate to 0.01%

The old default, 1%, was high enough that in a uniform random write workload,
we ended up needing to read in most of the key blocks even with bloom filters
enabled. On a 5 node cluster, after inserting a few billion rows, the write
throughput dropped dramatically as every batch of writes was seeking and
reading keys off disk.

In testing on the same cluster, changing the FP rate to 0.01% improved the
throughput dramatically by reducing the random reads coming off disk. The
cost is a 2x increase in bloom filter size (20 bits per key vs 10) but
20 bits is still a small percentage compared to typical row key sizes
in target applications.

Of course if an application has no random write characteristics and really
cares about disk space, this can always be flipped back.

Change-Id: I99bdd6298349a5be5f1fc3a666fe04305699e293
---
M docs/release_notes.adoc
M src/kudu/tablet/tablet.cc
2 files changed, 10 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I99bdd6298349a5be5f1fc3a666fe04305699e293
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 

[kudu-CR] locks: add new read-write mutex

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: locks: add new read-write mutex
..


Patch Set 5: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/2075/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] locks: change kudu::shared lock constructor to pass by ref

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: locks: change kudu::shared_lock constructor to pass by ref
..


Patch Set 5: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/2076/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e25a4f519f5cc09f08f8aeda5284eabdadd0c46
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] locks: add new read-write mutex

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: locks: add new read-write mutex
..


Patch Set 4:

(1 comment)

> (1 comment)
 > 
 > LGTM with some concern regarding error detection if something goes
 > wrong.
 > 
 > So, we are about to catch some those non-run-time issues like
 > trying to acquire the lock held by the same thread, etc. in debug
 > mode, which is OK.
 > 
 > Do we expect to catch some run-time issues like lack of shared
 > memory to accommodate a new lock, etc.?  I.e. we are not
 > propagating errors from pthread_rwlock_xxx() functions in release
 > mode at all (if I'm not missing something).   Does it make sense to
 > throw some sort of exception for run-time errors (like
 > std::runtime_error)?

It probably does (I'm not 100% sure, would need to give it more thought), but 
I'd rather keep that out of scope of this patch. The behavior of this new lock 
is consistent with that of our other pthread-based locks.

If we're going to address the issue of runtime checking of pthread return 
values, I think it makes sense to do it across the board in a separate patch.

http://gerrit.cloudera.org:8080/#/c/3496/4/src/kudu/util/mutex.cc
File src/kudu/util/mutex.cc:

Line 92:   ; // NOLINT(whitespace/semicolon)
> As Dan already mentioned: is it possible to keep the semicolon under the if
Yes, but I actually messed up when I published this version of the diff. I 
meant to pull the DCHECK_EQ() out of the NDEBUG block. See the next revision.

If it's pulled out, it's no longer possible to put the semicolon on the end of 
the line, at least not without duplicating the DCHECK_EQ().


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] locks: switch from boost::shared lock to kudu::shared lock

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: locks: switch from boost::shared_lock to kudu::shared_lock
..


Patch Set 5:

Build Started http://104.196.14.100/job/kudu-gerrit/2070/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523cc09345b50f7ad9e6af0180493774246a9bf8
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] thirdparty: add boost and switch to header-only build

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: thirdparty: add boost and switch to header-only build
..


Patch Set 4:

Build Started http://104.196.14.100/job/kudu-gerrit/2071/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id5b73a8e2a86099429b6032023a01a0da7b02371
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] locks: switch from boost::shared mutex to new read-write mutex

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: locks: switch from boost::shared_mutex to new read-write mutex
..


Patch Set 5:

Build Started http://104.196.14.100/job/kudu-gerrit/2069/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b4b1df267468dc8c88c3de8e0345c61900576de
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] locks: add new read-write mutex

[Adar Dembo (Code Review)]

Hello Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: locks: add new read-write mutex
..

locks: add new read-write mutex

The new mutex is a thin wrapper around pthread read/write locks. It has no
features of which to speak: no debugging hooks, no optimizations, nothing.

Take these rwlock-perf results with a grain of salt; they only test
read/read contention, not read/write or write/write contention. The values
are millions of cycles.

num_threads laptop_old laptop_new ve0518_old ve0518_new
---
1   26183 18982
2   1604   644914730
3   1504   9172204   1567
4   2398   1792   3185   2162
5   3210   2179   3943   2308
6   4070   2696   4135   2515
7   4741   3253   4557   2732
8   5457   3853   5114   3145

Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
---
M src/kudu/experiments/rwlock-perf.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/mutex.cc
A src/kudu/util/rw_mutex.cc
A src/kudu/util/rw_mutex.h
5 files changed, 122 insertions(+), 5 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-1496. NoLeaderMasterFoundException are mishandled

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has submitted this change and it was merged.

Change subject: KUDU-1496. NoLeaderMasterFoundException are mishandled
..


KUDU-1496. NoLeaderMasterFoundException are mishandled

This is a regression from 0c8f72b4c5d637dba4c0c0da4f55ea918c287d88.

Once we have multi-master unit tests re-enabled, we'll have this
covered.

Change-Id: Ie33410bdcfcc1bf9d3afb559a01f50538d7e10b5
Reviewed-on: http://gerrit.cloudera.org:8080/3516
Reviewed-by: Adar Dembo 
Reviewed-by: Dan Burkert 
Tested-by: Jean-Daniel Cryans 
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
1 file changed, 6 insertions(+), 5 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Jean-Daniel Cryans: Verified
  Adar Dembo: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie33410bdcfcc1bf9d3afb559a01f50538d7e10b5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Binglin Chang 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 

[kudu-CR] KUDU-1496. NoLeaderMasterFoundException are mishandled

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-1496. NoLeaderMasterFoundException are mishandled
..


Patch Set 1: Verified+1

Forcing +1 due to current messed up workspace.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie33410bdcfcc1bf9d3afb559a01f50538d7e10b5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Binglin Chang 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-HasComments: No

[kudu-CR] KUDU-1496. NoLeaderMasterFoundException are mishandled

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1496. NoLeaderMasterFoundException are mishandled
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie33410bdcfcc1bf9d3afb559a01f50538d7e10b5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Binglin Chang 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] thirdparty: add boost and switch to header-only build

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: thirdparty: add boost and switch to header-only build
..


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/2066/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id5b73a8e2a86099429b6032023a01a0da7b02371
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] locks: switch from boost::shared mutex to new read-write mutex

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: locks: switch from boost::shared_mutex to new read-write mutex
..


Patch Set 4:

Build Started http://104.196.14.100/job/kudu-gerrit/2064/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b4b1df267468dc8c88c3de8e0345c61900576de
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] locks: add new read-write mutex

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: locks: add new read-write mutex
..


Patch Set 4:

Build Started http://104.196.14.100/job/kudu-gerrit/2063/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] locks: switch from boost::shared lock to kudu::shared lock

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: locks: switch from boost::shared_lock to kudu::shared_lock
..


Patch Set 4:

Build Started http://104.196.14.100/job/kudu-gerrit/2065/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I523cc09345b50f7ad9e6af0180493774246a9bf8
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] locks: change kudu::shared lock constructor to pass by ref

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: locks: change kudu::shared_lock constructor to pass by ref
..


Patch Set 4:

Build Started http://104.196.14.100/job/kudu-gerrit/2062/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e25a4f519f5cc09f08f8aeda5284eabdadd0c46
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] locks: add new read-write mutex

[Adar Dembo (Code Review)]

Hello Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: locks: add new read-write mutex
..

locks: add new read-write mutex

The new mutex is a thin wrapper around pthread read/write locks. It has no
features of which to speak: no debugging hooks, no optimizations, nothing.

Take these rwlock-perf results with a grain of salt; they only test
read/read contention, not read/write or write/write contention. The values
are millions of cycles.

num_threads laptop_old laptop_new ve0518_old ve0518_new
---
1   26183 18982
2   1604   644914730
3   1504   9172204   1567
4   2398   1792   3185   2162
5   3210   2179   3943   2308
6   4070   2696   4135   2515
7   4741   3253   4557   2732
8   5457   3853   5114   3145

Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
---
M src/kudu/experiments/rwlock-perf.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/mutex.cc
A src/kudu/util/rw_mutex.cc
A src/kudu/util/rw_mutex.h
5 files changed, 122 insertions(+), 5 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] thirdparty: add boost and switch to header-only build

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: thirdparty: add boost and switch to header-only build
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id5b73a8e2a86099429b6032023a01a0da7b02371
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] KUDU-1496. NoLeaderMasterFoundException are mishandled

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: KUDU-1496. NoLeaderMasterFoundException are mishandled
..


Patch Set 1:

(1 comment)

> Oops.
 > 
 > Outside of multi-master, this only manifests when the sole master
 > is booting up, right? That would explain why it happens often in
 > our unit tests.

Yup, I just didn't want to write tests that we already have.

http://gerrit.cloudera.org:8080/#/c/3516/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:

Line 814: return d;
> is it important that the getDeferred call happen before delayedSendRpcToTab
If by some chance the RPC completed before the return is done, we'd be sending 
back a new Deferred. Almost impossible to run into, but hey...


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie33410bdcfcc1bf9d3afb559a01f50538d7e10b5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Binglin Chang 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1496. NoLeaderMasterFoundException are mishandled

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: KUDU-1496. NoLeaderMasterFoundException are mishandled
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3516/1/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java:

Line 814: return d;
is it important that the getDeferred call happen before delayedSendRpcToTablet? 
 If not I would suggest directly returning request.getDeferred().


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie33410bdcfcc1bf9d3afb559a01f50538d7e10b5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Binglin Chang 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1496. NoLeaderMasterFoundException are mishandled

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1496. NoLeaderMasterFoundException are mishandled
..


Patch Set 1: Code-Review+2

Oops.

Outside of multi-master, this only manifests when the sole master is booting 
up, right? That would explain why it happens often in our unit tests.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie33410bdcfcc1bf9d3afb559a01f50538d7e10b5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Binglin Chang 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] KUDU-1496. NoLeaderMasterFoundException are mishandled

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-1496. NoLeaderMasterFoundException are mishandled
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/2061/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie33410bdcfcc1bf9d3afb559a01f50538d7e10b5
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Add a ToString() method to Proxy

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Add a ToString() method to Proxy
..


Patch Set 1:

would be nice to add a new assertion to one of the RPC layer tests which calls 
this

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1e158db09e6e3c188b2725424681187a4b8c72e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR](gh-pages) Add 6/27 weekly update

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Add 6/27 weekly update
..


Patch Set 2: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6ce4bf6e02a5481bf3e032d3345d7afc93c244a2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR](gh-pages) Add 6/27 weekly update

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: Add 6/27 weekly update
..


Add 6/27 weekly update

Change-Id: I6ce4bf6e02a5481bf3e032d3345d7afc93c244a2
Reviewed-on: http://gerrit.cloudera.org:8080/3515
Reviewed-by: Jean-Daniel Cryans 
Tested-by: Todd Lipcon 
---
A _posts/2016-06-27-weekly-update.md
1 file changed, 91 insertions(+), 0 deletions(-)

Approvals:
  Jean-Daniel Cryans: Looks good to me, approved
  Todd Lipcon: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I6ce4bf6e02a5481bf3e032d3345d7afc93c244a2
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR](gh-pages) Add 6/27 weekly update

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: Add 6/27 weekly update
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6ce4bf6e02a5481bf3e032d3345d7afc93c244a2
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-HasComments: No

[kudu-CR](gh-pages) Add 6/27 weekly update

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: Add 6/27 weekly update
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3515/1/_posts/2016-06-27-weekly-update.md
File _posts/2016-06-27-weekly-update.md:

PS1, Line 24: replication
replicating?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6ce4bf6e02a5481bf3e032d3345d7afc93c244a2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-HasComments: Yes

[kudu-CR] thirdparty: add boost and switch to header-only build

[Adar Dembo (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: thirdparty: add boost and switch to header-only build
..

thirdparty: add boost and switch to header-only build

The addition of boost to thirdparty is bound to make some squeamish, so
here's my rationale:
1. My original goal was to enable the TSAN build on Ubuntu 16.04, a distro
   that has switched to the new gcc5 ABI. This renders the system boost
   installation unusable as it can't link against our TSAN-instrumented
   libstdc++. To be fair, switching to a header-only build has eliminated
   this problem.
2. Boost is effectively the last remaining dependency that isn't explicitly
   managed as a thirdparty dependency. The only exceptions are glibc,
   libstdc++, and libsasl, all of which should not be managed due to their
   interactions with other ABIs. In my opinion, Kudu should manage as many
   dependencies as possible in this way.
3. Doing this means Kudu developers will no longer need to install boost
   system packages to build Kudu.
4. Historically, Kudu has permitted the use of any boost version even
   though some versions needed patches to build with a new compiler. IIRC,
   https://svn.boost.org/trac/boost/ticket/6165 was one such instance.
5. For the boost logic that Kudu does use, we'll know exactly which features
   exist and which do not. There'll be no more version-based ambiguity.
6. Doing this means we can drop the odd boost_uuid appendage.
7. This patch by no means implies that boost is to stay in Kudu forever.
   We're already at the point where we can get by with boost headers alone.
   Once we replace the remaining bits (i.e. optional and intrusive lists
   come to mind), we can jettison it altogether.

With that out of the way, here are the details:
- Inclusion of boost as a thirdparty dependency. I spent some time fighting
  with the boost build to get it to copy the headers without building
  anything; eventually I gave up and used rsync instead.
- Removal of boost components from Kudu's cmake logic. This netted a nice
  cleanup of cmake code.
- Removal of boost handling from documentation and other random places.
- Removal of boost_uuid.

Change-Id: Id5b73a8e2a86099429b6032023a01a0da7b02371
---
M CMakeLists.txt
M LICENSE.txt
M build-support/dist_test.py
M build-support/release/rat_exclude_files.txt
M docs/contributing.adoc
M docs/installation.adoc
M src/kudu/util/CMakeLists.txt
M thirdparty/.gitignore
M thirdparty/LICENSE.txt
D thirdparty/boost_uuid/LICENSE.txt
D thirdparty/boost_uuid/boost/uuid/name_generator.hpp
D thirdparty/boost_uuid/boost/uuid/nil_generator.hpp
D thirdparty/boost_uuid/boost/uuid/random_generator.hpp
D thirdparty/boost_uuid/boost/uuid/seed_rng.hpp
D thirdparty/boost_uuid/boost/uuid/sha1.hpp
D thirdparty/boost_uuid/boost/uuid/string_generator.hpp
D thirdparty/boost_uuid/boost/uuid/uuid.hpp
D thirdparty/boost_uuid/boost/uuid/uuid_generators.hpp
D thirdparty/boost_uuid/boost/uuid/uuid_io.hpp
D thirdparty/boost_uuid/boost/uuid/uuid_serialize.hpp
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
A thirdparty/patches/boost-issue-12179-fix-compilation-errors.patch
M thirdparty/preflight.py
M thirdparty/vars.sh
26 files changed, 98 insertions(+), 1,558 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id5b73a8e2a86099429b6032023a01a0da7b02371
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] locks: change kudu::shared lock constructor to pass by ref

[Adar Dembo (Code Review)]

Hello Dan Burkert, Kudu Jenkins,

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

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

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

Change subject: locks: change kudu::shared_lock constructor to pass by ref
..

locks: change kudu::shared_lock constructor to pass by ref

This is less Kudu-style friendly, but it'll make the (eventual) transition
to the C++14 std::shared_lock smoother, and it's also more consistent with
std::lock_guard.

Change-Id: I1e25a4f519f5cc09f08f8aeda5284eabdadd0c46
---
M src/kudu/client/meta_cache.cc
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/rpc/messenger.cc
M src/kudu/rpc/messenger.h
M src/kudu/rpc/rpcz_store.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/util/locks.h
9 files changed, 20 insertions(+), 20 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e25a4f519f5cc09f08f8aeda5284eabdadd0c46
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] thirdparty: add boost and switch to header-only build

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: thirdparty: add boost and switch to header-only build
..


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/2060/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id5b73a8e2a86099429b6032023a01a0da7b02371
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR](gh-pages) Add 6/27 weekly update

[Todd Lipcon (Code Review)]

Hello Jean-Daniel Cryans,

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

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

to review the following change.

Change subject: Add 6/27 weekly update
..

Add 6/27 weekly update

Change-Id: I6ce4bf6e02a5481bf3e032d3345d7afc93c244a2
---
A _posts/2016-06-27-weekly-update.md
1 file changed, 91 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6ce4bf6e02a5481bf3e032d3345d7afc93c244a2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Jean-Daniel Cryans 

[kudu-CR] Implement kudu::optional replacement for boost::optional

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Implement kudu::optional replacement for boost::optional
..


Patch Set 1:

yea, ASAN does insert poisoning between variables on the stack: 
http://llvm.org/docs/doxygen/html/ASanStackFrameLayout_8cpp_source.html

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib80c35cc9a4712572f85eeb7717e17869cd5e081
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Fix flakiness in RaftConsensusITest.TestMasterNotifiedOnConfigChange

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Fix flakiness in 
RaftConsensusITest.TestMasterNotifiedOnConfigChange
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb5875e8d4200b0058cf7e7c4ee04771b91d2a24
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Fix flakiness in RaftConsensusITest.TestMasterNotifiedOnConfigChange

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Fix flakiness in 
RaftConsensusITest.TestMasterNotifiedOnConfigChange
..


Patch Set 1:

A java test failed due to https://issues.apache.org/jira/browse/KUDU-1496 
(unrelated)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb5875e8d4200b0058cf7e7c4ee04771b91d2a24
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Implement kudu::optional replacement for boost::optional

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Implement kudu::optional replacement for boost::optional
..


Patch Set 1:

> Also not sure of the purpose of the extra red zones (ASAN
 > would already insert those around the object, right?)

I didn't think ASAN did this for stack-allocated objects. Does it? If so, the 
red zones should be removed from this impl.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib80c35cc9a4712572f85eeb7717e17869cd5e081
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Implement kudu::optional replacement for boost::optional

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Implement kudu::optional replacement for boost::optional
..


Patch Set 1:

> I would like some careful eyes on this before we merge it, as the
 > move semantics related stuff that is being done is at the edge of
 > my understanding of C++11.

Not presupposing that we will merge it, but if we do.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib80c35cc9a4712572f85eeb7717e17869cd5e081
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Allow to force-override color diagnostics

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Allow to force-override color diagnostics
..


Patch Set 1: Code-Review+2

Sounds like there's a reasonable use case for it.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7c19651df6d74753b0d4e8ac85ff98326dee7b10
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] thirdparty: add boost and switch to header-only build

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: thirdparty: add boost and switch to header-only build
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3500/1/docs/contributing.adoc
File docs/contributing.adoc:

Line 151: We are in the process of removing all remaining `boost` dependencies 
from the
> Yes, but additionally I think boost::optional is a powerful tool for writin
Alright, then I'll update this to say something to the effect of:
1. We're actively trying to remove boost.
2. We're using boost in a header-only capacity right now; don't introduce any 
library dependencies.
3. Please don't introduce dependencies on new boost header-only features.
4. You can use existing boost header-only features on which we already depend 
(e.g. boost::optional, boost::bind). These features may be removed in the 
future; when using boost, please be mindful of the "current state" of the world 
(e.g. boost::bind might be gone soon).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id5b73a8e2a86099429b6032023a01a0da7b02371
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] ts recovery-itest: fix flakiness in TestRestartWithPendingCommitFromFailedOp

[Todd Lipcon (Code Review)]

Hello Adar Dembo,

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

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

to review the following change.

Change subject: ts_recovery-itest: fix flakiness in 
TestRestartWithPendingCommitFromFailedOp
..

ts_recovery-itest: fix flakiness in TestRestartWithPendingCommitFromFailedOp

This test seems to be flaky due to not waiting long enough for the TS to crash.
I tried to repro on dist-test, but the GCE test slaves are fast enough that it
passed 1000/1000. On EC2 apparently things are a little slower so 10 seconds
isn't enough time to guarantee that the injected fault fires.

Change-Id: I60db2acf7642aa6eb2c850032a7dc9639b6ae40e
---
M src/kudu/integration-tests/ts_recovery-itest.cc
1 file changed, 1 insertion(+), 1 deletion(-)


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

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

[kudu-CR] ts recovery-itest: fix flakiness in TestRestartWithPendingCommitFromFailedOp

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: ts_recovery-itest: fix flakiness in 
TestRestartWithPendingCommitFromFailedOp
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/2055/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I60db2acf7642aa6eb2c850032a7dc9639b6ae40e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Implement kudu::optional replacement for boost::optional

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Implement kudu::optional replacement for boost::optional
..


Patch Set 1:

(1 comment)

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

Line 237:   const T& GetValueAsConstRef() const& { return 
*GetValueAsConstPtr(); }
> So there's a difference between a "const" method and a "const&" method? Did
The & allows it to bind to an rvalue object as well as an lvalue object.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib80c35cc9a4712572f85eeb7717e17869cd5e081
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Implement kudu::optional replacement for boost::optional

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Implement kudu::optional replacement for boost::optional
..


Patch Set 1:

> I don't see the purpose of taking on this extra code. Code's a
 > liability, not an asset, and all that.

This is meant to completely replace our usage of boost::optional so it should 
be net zero in terms of new code used vs. old code dropped, or even net 
negative given that I don't think Mike reimplemented all of boost::optional. 
Besides, you've been open to incorporating Chromium code in the past to replace 
boost usage (e.g. Chromium callback/bind). Is this terribly different?

I think there are reasonable counter-arguments to be made (e.g. boost::optional 
was probably written/tested by people with deeper C++ knowledge than 
ourselves), but I don't think this one is that convincing.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib80c35cc9a4712572f85eeb7717e17869cd5e081
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Implement kudu::optional replacement for boost::optional

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Implement kudu::optional replacement for boost::optional
..


Patch Set 1:

(3 comments)

I only skimmed this; I think a real review requires a level of C++ knowledge 
that I don't think I have.

http://gerrit.cloudera.org:8080/#/c/3512/1/src/kudu/gutil/dynamic_annotations.h
File src/kudu/gutil/dynamic_annotations.h:

Line 64: #include  // For size_t.
Technically size_t is defined in stddef.h (or cstddef, if you prefer a C++ 
header).


http://gerrit.cloudera.org:8080/#/c/3512/1/src/kudu/util/CMakeLists.txt
File src/kudu/util/CMakeLists.txt:

Line 297: ADD_KUDU_TEST(optional-test)
Nit: precedes os-util-test alphabetically.


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

Line 237:   const T& GetValueAsConstRef() const& { return 
*GetValueAsConstPtr(); }
So there's a difference between a "const" method and a "const&" method? Didn't 
know that. What is the difference?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib80c35cc9a4712572f85eeb7717e17869cd5e081
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Migrate from boost::optional to kudu::optional

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Migrate from boost::optional to kudu::optional
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3513/1/src/kudu/tablet/memrowset.cc
File src/kudu/tablet/memrowset.cc:

Line 411: exclusive_upper_bound_ = 
spec->exclusive_upper_bound_key()->encoded_key();
Need to check w/ Todd on this but I think it should be fine to do it.


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

Line 670: return; }
Oops, inadvertent change, need to restore the newline before this brace.


http://gerrit.cloudera.org:8080/#/c/3513/1/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

Line 993:   Status delete_status = DeleteTabletData(meta, 
TABLET_DATA_TOMBSTONED, optional());
This should probably be replaced with nullopt


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia46ba817d507af8b26add764dc3859364d15e0da
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] Fix flakiness in RaftConsensusITest.TestMasterNotifiedOnConfigChange

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Fix flakiness in 
RaftConsensusITest.TestMasterNotifiedOnConfigChange
..


Patch Set 1: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/2054/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb5875e8d4200b0058cf7e7c4ee04771b91d2a24
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] Allow to force-override color diagnostics

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Allow to force-override color diagnostics
..


Patch Set 1:

I use something like this locally -- when I run 'ninja', it sometimes re-runs 
cmake for me, but the output is a pipe into ninja, rather than a TTY, which 
means that cmake will decide not to use color.

My workaround is to put it in my ccache-clang wrapper though:

#!/bin/bash -e

if [ -n "$CLANG_ALWAYS_COLOR" ] || test -t 2 ; then
  color_flags="-fcolor-diagnostics"
fi

CCACHE_CPP2=yes exec ccache clang++ -Qunused-arguments $color_flags "$@"

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7c19651df6d74753b0d4e8ac85ff98326dee7b10
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Implement kudu::optional replacement for boost::optional

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Implement kudu::optional replacement for boost::optional
..


Patch Set 1: Code-Review-1

I don't see the purpose of taking on this extra code. Code's a liability, not 
an asset, and all that.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib80c35cc9a4712572f85eeb7717e17869cd5e081
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Allow to force-override color diagnostics

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Allow to force-override color diagnostics
..


Patch Set 1:

(1 comment)

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

Line 230:   # We also provide a manual override as 
-DKUDU_FORCE_COLOR_DIAGNOSTICS=1.
> Curious: what's the motivation for the override? How is the existing heuris
I looked into it a little bit. I'm not exactly sure why it's failing for me. I 
wonder if it has to do with bash aliases and functions. But AFAICT I am not 
redirecting stderr.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7c19651df6d74753b0d4e8ac85ff98326dee7b10
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] Update docs on how to run gcovr

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

Change subject: Update docs on how to run gcovr
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3508/1/README.adoc
File README.adoc:

Line 241: $ mkdir cov_html
> Hmm, probably don't need this anymore.
ah, right


Line 244:   --gcov-executable=$(pwd)/../../build-support/llvm-gcov-wrapper \
> Why do we need $(pwd)? We're in build/coverage; given the "blessed" build l
I know it's lame but it doesn't work without an absolute path. I'm sure it's a 
bug in gcovr. This is the easiest way.


Line 245:   --html --html-details -o coverage.html
> What happens if you run without -o?
I'm not sure. I'll try it out.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I494136e20452b76572d753b54fc7a095aa54a69b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] Migrate from boost::optional to kudu::optional

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Migrate from boost::optional to kudu::optional
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/2053/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia46ba817d507af8b26add764dc3859364d15e0da
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] thirdparty: add boost and switch to header-only build

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: thirdparty: add boost and switch to header-only build
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3500/1/docs/contributing.adoc
File docs/contributing.adoc:

Line 151: We are in the process of removing all remaining `boost` dependencies 
from the
> I really was hoping to avoid introducing new Boost usage, so that we stand 
Yes, but additionally I think boost::optional is a powerful tool for writing 
higher quality code, and its use should be encouraged wherever it makes sense 
from a code simplicity/maintainability perspective.  Optional is especially 
important when working with value types, as is often possible with the new move 
semantics, and unlocks simpler and more performant solutions than the existing 
tools (e.g. nullable unique_ptr) in many cases.  Basically, I think Optional is 
a great abstraction and to discourage its use because of a minor distaste of 
boost is going to do more harm than good.

I don't care as much about intrusive, and would agree that we should not 
encourage it, only that it should be available in the very few cases where it 
is justified.  I doubt it will get used again any time soon, so it's a bit of a 
non-issue.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id5b73a8e2a86099429b6032023a01a0da7b02371
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Implement kudu::optional replacement for boost::optional

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Implement kudu::optional replacement for boost::optional
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/2052/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib80c35cc9a4712572f85eeb7717e17869cd5e081
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Implement kudu::optional replacement for boost::optional

[Mike Percy (Code Review)]

Hello Adar Dembo,

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

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

to review the following change.

Change subject: Implement kudu::optional replacement for boost::optional
..

Implement kudu::optional replacement for boost::optional

This class attempts to implement as much of the C++17 specification of
std::optional as is reasonably simple to provide in C++11.

This class does not throw exceptions and value() is not implemented.

Change-Id: Ib80c35cc9a4712572f85eeb7717e17869cd5e081
---
M src/kudu/gutil/dynamic_annotations.h
M src/kudu/util/CMakeLists.txt
A src/kudu/util/optional-test.cc
A src/kudu/util/optional.h
A src/kudu/util/optional_fwd.h
5 files changed, 518 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib80c35cc9a4712572f85eeb7717e17869cd5e081
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 

[kudu-CR] Migrate from boost::optional to kudu::optional

[Mike Percy (Code Review)]

Hello Adar Dembo,

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

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

to review the following change.

Change subject: Migrate from boost::optional to kudu::optional
..

Migrate from boost::optional to kudu::optional

Change-Id: Ia46ba817d507af8b26add764dc3859364d15e0da
---
M src/kudu/client/client.cc
M src/kudu/client/scan_predicate.cc
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/table_alterer-internal.cc
M src/kudu/client/table_alterer-internal.h
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M src/kudu/common/partition-test.cc
M src/kudu/common/partition_pruner-test.cc
M src/kudu/common/partition_pruner.cc
M src/kudu/common/wire_protocol.cc
M src/kudu/common/wire_protocol.h
M src/kudu/consensus/consensus.h
M src/kudu/consensus/raft_consensus.cc
M src/kudu/consensus/raft_consensus.h
M src/kudu/integration-tests/client_failover-itest.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/delete_table-test.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/remote_bootstrap-itest.cc
M src/kudu/integration-tests/tablet_replacement-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/rpc/service_pool.cc
M src/kudu/rpc/service_queue-test.cc
M src/kudu/rpc/service_queue.cc
M src/kudu/rpc/service_queue.h
M src/kudu/tablet/composite-pushdown-test.cc
M src/kudu/tablet/memrowset.cc
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tools/kudu-admin.cc
M src/kudu/tserver/tablet_peer_lookup.h
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
38 files changed, 168 insertions(+), 173 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia46ba817d507af8b26add764dc3859364d15e0da
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 

[kudu-CR] Update docs on how to run gcovr

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Update docs on how to run gcovr
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3508/1/README.adoc
File README.adoc:

Line 241: $ mkdir cov_html
Hmm, probably don't need this anymore.


Line 244:   --gcov-executable=$(pwd)/../../build-support/llvm-gcov-wrapper \
Why do we need $(pwd)? We're in build/coverage; given the "blessed" build 
layout, isn't that guaranteed to be two subdirs deeper than build-support?


Line 245:   --html --html-details -o coverage.html
What happens if you run without -o?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I494136e20452b76572d753b54fc7a095aa54a69b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] thirdparty: add boost and switch to header-only build

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: thirdparty: add boost and switch to header-only build
..


Patch Set 1:

(3 comments)

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

Line 18:libstdc++, and libsasl, all of which should not be managed due to 
their
> and openssl, although the point still stands.
Right, though I explicitly omitted openssl because, at least for now, you 
really can't configure Kudu to use it.


PS1, Line 22: Some distros (e.g. SLES12) don't offer
:system packages
> The specific issue with SLES 12 (and I would assume the majority of the sma
Ah, I misread the blurb in installation.adoc. Will update.


http://gerrit.cloudera.org:8080/#/c/3500/1/docs/contributing.adoc
File docs/contributing.adoc:

Line 151: We are in the process of removing all remaining `boost` dependencies 
from the
> We still have 'approved' boost libraries.  I would vote to keep the approve
I really was hoping to avoid introducing new Boost usage, so that we stand a 
chance of removing the whole thing some day.

Is your point that adding a new usage of e.g. boost::optional isn't a big deal 
because, if/when we do replace it, updating an additional call-site is no big 
deal?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id5b73a8e2a86099429b6032023a01a0da7b02371
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] locks: stop using errno around base::NumCPUs and base::MaxCPUIndex

[Adar Dembo (Code Review)]

Adar Dembo has submitted this change and it was merged.

Change subject: locks: stop using errno around base::NumCPUs and 
base::MaxCPUIndex
..


locks: stop using errno around base::NumCPUs and base::MaxCPUIndex

It's not clear why we ever did this. InitializeSystemInfo() doesn't
explicitly set errno itself, nor does it clear it in the event of an ignored
error (e.g. ReadIntFromFile() on a file that doesn't exist).

Without this fix, I consistently get the following when I run rwlock-perf:

Test   Threads  Cycles
--
WARNING: Logging before InitGoogleLogging() is written to STDERR
F0625 12:20:24.620546 12138 locks.h:163] Check failed: (*__errno_location ()) 
== 0 (2 vs. 0) No such file or directory
*** Check failure stack trace: ***

Amusingly, I only see this in release builds because in a dynamically linked
rwperf-test the very first call to InitializeSystemInfo() (subsequent calls
are cached) takes place in a location that doesn't check errno upon return.

Change-Id: I7c048a5a541bd3bfd54f8ac4acc1dd9e379c47ad
Reviewed-on: http://gerrit.cloudera.org:8080/3495
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/consensus/log_index.cc
M src/kudu/experiments/rwlock-perf.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/util/locks.h
4 files changed, 13 insertions(+), 14 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I7c048a5a541bd3bfd54f8ac4acc1dd9e379c47ad
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Fix flakiness in RaftConsensusITest.TestMasterNotifiedOnConfigChange

[Todd Lipcon (Code Review)]

Hello Mike Percy,

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

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

to review the following change.

Change subject: Fix flakiness in 
RaftConsensusITest.TestMasterNotifiedOnConfigChange
..

Fix flakiness in RaftConsensusITest.TestMasterNotifiedOnConfigChange

This test was supposed to wait until the master had committed
a given operation, but in fact was only waiting until all of the
servers had _replicated_ the operation. In TSAN builds where things
ran slower, this caused it to be about 7% flaky.

The fix is to simply wait for the op to be committed, not just replicated.
With the fix, I looped it 100 times and it passed.

Change-Id: Ibb5875e8d4200b0058cf7e7c4ee04771b91d2a24
---
M src/kudu/integration-tests/raft_consensus-itest.cc
1 file changed, 1 insertion(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibb5875e8d4200b0058cf7e7c4ee04771b91d2a24
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Mike Percy 

[kudu-CR] locks: add new read-write mutex

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: locks: add new read-write mutex
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3496/2/src/kudu/util/rw_mutex.cc
File src/kudu/util/rw_mutex.cc:

Line 26:   DCHECK_EQ(0, rv) << strerror(rv);
> Yes it does. As do the unused 'rv' in Mutex and ConditionVariable.
Quick note: these don't cause unused warnings, at least not with gcc 5.3. There 
was one warning in mutex.cc; I've fixed it.


Line 34:   pthread_rwlock_init(_handle_, NULL);
> Does it make sense to check for return value here as well?
The manpage says it should never return non-zero, but I've added the check 
anyway; it doesn't hurt and is more defensive.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5462e69291fb9498ebd4aaa1728c64658667aa4b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Fix flakiness in RaftConsensusITest.TestMasterNotifiedOnConfigChange

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Fix flakiness in 
RaftConsensusITest.TestMasterNotifiedOnConfigChange
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/2051/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibb5875e8d4200b0058cf7e7c4ee04771b91d2a24
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR](gh-pages) kudu flume sink blog post

[Ara Ebrahimi (Code Review)]

Ara Ebrahimi has uploaded a new change for review.

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

Change subject: kudu flume sink blog post
..

kudu flume sink blog post

Change-Id: I810146ab24c88bc6cc562d81746b9bf5303396ed
---
A _posts/2016-07-06-flume.md
1 file changed, 141 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I810146ab24c88bc6cc562d81746b9bf5303396ed
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Ara Ebrahimi 

[kudu-CR] Integrate the result tracker with writes

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Integrate the result tracker with writes
..


Patch Set 7: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/2050/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1fa2f8db33653960f4749237b8993baba0929893
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a test for the integration of RequestTracker with the client and ResultTracker with the server

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a test for the integration of RequestTracker with the 
client and ResultTracker with the server
..


Patch Set 6: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/2048/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic193dd5a547fe2a107a2ab1625f93fa0c19cc61b
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Integrate the request tracker with the client

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Integrate the request tracker with the client
..


Patch Set 14:

Build Started http://104.196.14.100/job/kudu-gerrit/2047/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I94207c294452fcbdb3a7901fdb702674d47553ee
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Integrate the ResultTracker into the rpc subsystem

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Integrate the ResultTracker into the rpc subsystem
..

Integrate the ResultTracker into the rpc subsystem

This integrates the ResultTracker into the rpc subsystem by allowing
to specify a new method option when defining rpc service methods.
When this method option is specificed _and_ the call's rpc header
has a RequestIdPB the results for the call will be tracked and the
call may have exactly once semantics.

This allows to have the simplest form of exactly once semantics for
single server calls. Of course limitations apply, like response
persistence across restarts is not supported, neither is propagating/rebuilding
responses to/on replicas for replicated calls. If support for this
is needed it will have to be done ad-hoc, case by case.

A follow up patch tests this in integration with the client.

Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6
---
M src/kudu/rpc/CMakeLists.txt
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/protoc-gen-krpc.cc
M src/kudu/rpc/rpc_context.cc
M src/kudu/rpc/rpc_context.h
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/rpc/service_if.h
8 files changed, 138 insertions(+), 42 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Integrate the ResultTracker into the rpc subsystem

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Integrate the ResultTracker into the rpc subsystem
..


Patch Set 16:

Build Started http://104.196.14.100/job/kudu-gerrit/2049/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Avoid missing 'override' keyword warnings in raft consensus-test.cc

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has submitted this change and it was merged.

Change subject: Avoid missing 'override' keyword warnings in 
raft_consensus-test.cc
..


Avoid missing 'override' keyword warnings in raft_consensus-test.cc

In this test we're using gmock and the compiler spews out warnings
that the mocked methods are missing the 'override' keyword.

This just makes it so that we ignore that warning in that file.

Change-Id: I687a05ed560003721d4a05bd7a01261e03bd77d9
Reviewed-on: http://gerrit.cloudera.org:8080/3289
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon 
---
M src/kudu/consensus/CMakeLists.txt
1 file changed, 8 insertions(+), 0 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I687a05ed560003721d4a05bd7a01261e03bd77d9
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Allow to force-override color diagnostics

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Allow to force-override color diagnostics
..


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/2046/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7c19651df6d74753b0d4e8ac85ff98326dee7b10
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Update docs on how to run gcovr

[Mike Percy (Code Review)]

Mike Percy has uploaded a new change for review.

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

Change subject: Update docs on how to run gcovr
..

Update docs on how to run gcovr

Change-Id: I494136e20452b76572d753b54fc7a095aa54a69b
---
M README.adoc
1 file changed, 3 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I494136e20452b76572d753b54fc7a095aa54a69b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 

[kudu-CR] Avoid missing 'override' keyword warnings in raft consensus-test.cc

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Avoid missing 'override' keyword warnings in 
raft_consensus-test.cc
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I687a05ed560003721d4a05bd7a01261e03bd77d9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] locks: change kudu::shared lock constructor to pass by ref

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: locks: change kudu::shared_lock constructor to pass by ref
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3497/2/src/kudu/util/locks.h
File src/kudu/util/locks.h:

Line 242: : m_(DCHECK_NOTNULL()) {
> this DCHECK seems a bit silly now (if we tried to create a null reference t
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e25a4f519f5cc09f08f8aeda5284eabdadd0c46
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Integrate the request tracker with the client

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Integrate the request tracker with the client
..


Patch Set 13: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/2044/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I94207c294452fcbdb3a7901fdb702674d47553ee
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Disable exactly once semantics by default and add a flag to enable it for tests

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Disable exactly once semantics by default and add a flag to 
enable it for tests
..


Patch Set 4: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/2043/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I77096be608afb31194f62f04a946bd3f42537a35
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Integrate the result tracker with writes

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Integrate the result tracker with writes
..

Integrate the result tracker with writes

This patch integrates the result tracker with write transactions,
including:
- Support for caching responses on tablet bootstrap.
- Support for caching responses for follower transactions.
- Handling of races between client originated and (previous?) leader
  originated transactions.
- An explanation of the interaction between the result tracker
  and the transaction driver in transaction_driver.h.

For integration tests, this patch removes the check that allowed
Status::AlreadyPresent() that we added as we didn't have support for
exactly once semantics. Without this check, in slow mode, some tests
like raft_consensus-itest would fail 100% of the time, due to rows
being inserted multiple times.

Because we'd have 100% failure rate without replay cache for
writes this patch doesn't include additional tests, which will
be pushed later, ad-hoc. The reasoning being that we'll benefit
from more coverage in the meanwhile.

Change-Id: I1fa2f8db33653960f4749237b8993baba0929893
---
M src/kudu/client/batcher.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/test_workload.cc
M src/kudu/integration-tests/test_workload.h
M src/kudu/master/sys_catalog.cc
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap-test.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/tablet_bootstrap.h
M src/kudu/tablet/tablet_peer-test.cc
M src/kudu/tablet/tablet_peer.cc
M src/kudu/tablet/tablet_peer.h
M src/kudu/tablet/transactions/transaction.h
M src/kudu/tablet/transactions/transaction_driver.cc
M src/kudu/tablet/transactions/transaction_driver.h
M src/kudu/tablet/transactions/write_transaction.cc
M src/kudu/tablet/transactions/write_transaction.h
M src/kudu/tools/insert-generated-rows.cc
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/remote_bootstrap_session-test.cc
M src/kudu/tserver/tablet_server.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
M src/kudu/tserver/tserver.proto
M src/kudu/tserver/tserver_service.proto
26 files changed, 403 insertions(+), 33 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1fa2f8db33653960f4749237b8993baba0929893
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Add a ToString() method to Proxy

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Add a ToString() method to Proxy
..


Patch Set 1:

(1 comment)

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

Line 110:   return StringPrintf("%s@%s", service_name_.c_str(), 
conn_id_.ToString().c_str());
Why do you need the c_str() calls? If it's a StringPrintf thing, maybe you can 
use strings::Substitute instead?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1e158db09e6e3c188b2725424681187a4b8c72e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Integrate the ResultTracker into the rpc subsystem

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Integrate the ResultTracker into the rpc subsystem
..


Patch Set 15: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/2041/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1d624810350feceefe244e0319e22cac241bf0d6
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Disable exactly once semantics by default and add a flag to enable it for tests

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Disable exactly once semantics by default and add a flag to 
enable it for tests
..


Patch Set 4: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/2040/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I77096be608afb31194f62f04a946bd3f42537a35
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a test for the integration of RequestTracker with the client and ResultTracker with the server

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a test for the integration of RequestTracker with the 
client and ResultTracker with the server
..


Patch Set 5: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/2039/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic193dd5a547fe2a107a2ab1625f93fa0c19cc61b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java-client] refactor AsyncKuduSession

[Dan Burkert (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [java-client] refactor AsyncKuduSession
..

[java-client] refactor AsyncKuduSession

This commit refactors how AsyncKuduSession handles buffering internally. Instead
of using a buffer per tablet and a separate list of operations in tablet
location lookup, we buffer all operations into a single large buffer, and then
dispatch into per-tablet batches during flush.

This result is simplified logic in AsyncKuduSession. AsyncKuduSession#apply is
no longer reentrant, and lookups no longer need to be tracked explicitly.

This patch was tested against YCSB to make sure that it does not result in
performance regressions.  With an increased buffer size of 5000 to match the 0.9
client's buffer size, the performance is largely the same. With this refactor
the effective batch size is much smaller since mutationBufferSize now refers to
the one buffer for all tablets instead of to a per-tablet buffer.

Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
---
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
M java/kudu-client/src/main/java/org/kududb/client/Batch.java
M java/kudu-client/src/main/java/org/kududb/client/Bytes.java
M java/kudu-client/src/main/java/org/kududb/client/Operation.java
M java/kudu-client/src/main/java/org/kududb/client/RowError.java
M java/kudu-client/src/main/java/org/kududb/util/AsyncUtil.java
M java/kudu-client/src/test/java/org/kududb/client/TestOperation.java
8 files changed, 593 insertions(+), 639 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] [java-client] refactor AsyncKuduSession

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3477/2/java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java
File java/kudu-client/src/main/java/org/kududb/client/AsyncKuduSession.java:

PS2, Line 396: Must not be concurrently accessed
> I'm still thrown off by this comment. Maybe:
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] [java-client] refactor AsyncKuduSession

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: [java-client] refactor AsyncKuduSession
..


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/2038/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ee6d029b1a56e254bfb9a870917883abeadb6b8
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Integrate the result tracker with writes

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Integrate the result tracker with writes
..


Patch Set 6:

Build Started http://104.196.14.100/job/kudu-gerrit/2036/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1fa2f8db33653960f4749237b8993baba0929893
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a test for the integration of RequestTracker with the client and ResultTracker with the server

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a test for the integration of RequestTracker with the 
client and ResultTracker with the server
..


Patch Set 5:

Build Started http://104.196.14.100/job/kudu-gerrit/2034/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic193dd5a547fe2a107a2ab1625f93fa0c19cc61b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] Disable exactly once semantics by default and add a flag to enable it for tests

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Disable exactly once semantics by default and add a flag to 
enable it for tests
..


Patch Set 4:

Build Started http://104.196.14.100/job/kudu-gerrit/2035/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I77096be608afb31194f62f04a946bd3f42537a35
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add a test for the integration of RequestTracker with the client and ResultTracker with the server

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: Add a test for the integration of RequestTracker with the 
client and ResultTracker with the server
..

Add a test for the integration of RequestTracker with the client and 
ResultTracker with the server

This adds a test for the integration of the RequestTracker in the client
and the ResultTracker in the server. Specifically it tests that RetriableRpc
works in combination with replayed results from the ResultTracker and that
multiple simultaneous (similar) RPCs from the client are not executed more
than once.

Change-Id: Ic193dd5a547fe2a107a2ab1625f93fa0c19cc61b
---
A src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rtest.proto
3 files changed, 366 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic193dd5a547fe2a107a2ab1625f93fa0c19cc61b
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Add a test for the integration of RequestTracker with the client and ResultTracker with the server

[Kudu Jenkins (Code Review)]

Kudu Jenkins has posted comments on this change.

Change subject: Add a test for the integration of RequestTracker with the 
client and ResultTracker with the server
..


Patch Set 4: -Verified

Build Started http://104.196.14.100/job/kudu-gerrit/2033/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic193dd5a547fe2a107a2ab1625f93fa0c19cc61b
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] locks: switch from boost::shared mutex to new read-write mutex

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: locks: switch from boost::shared_mutex to new read-write mutex
..


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b4b1df267468dc8c88c3de8e0345c61900576de
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No