date:20160718

Alexey Serbin has posted comments on this change.

Change subject: doxygen for C++ client API
..


Patch Set 9:

(1 comment)

> One thing we should add to this (it can be a follow-up commit).
 > 
 > It needs to be integrated into make_site.sh and the web site
 > itself.

Yes, certainly -- I will add that integration is a follow-up commit.

http://gerrit.cloudera.org:8080/#/c/3619/9/src/kudu/client/client.h
File src/kudu/client/client.h:

> High level formatting questions:
1.  It's a purely stylistic change, it's not related to doxygen requirements at 
all.  So, if there is some sort of implicit rule of using just one space before 
starting a new sentence, I'll revert it back.

2.  The idea was to have a brief (i.e. one sentence) a method/class summary, so 
no period would be needed since after @brief message there should be an empty 
line.  However, for parameters and return values there is no guarantee the 
description is brief, because it's the only description around for those.  So, 
@brief would not be ended with a period, but @return and @param would.  If that 
seems to be an inconsistency, let's end @brief message with a period as well.

3. It's because some methods were treated correspondingly after I got feedback 
from Todd in the kudu-dev mailing list :)  I.e., I removed @brief comments 
which were almost one-to-one copy of the @return comments.  I'll take care of 
the rest of those simple getters leaving only @return comment.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Misty Stanley-Jones 
Gerrit-HasComments: Yes

[kudu-CR] async background flush provision for C++ client

2016-07-18 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: async background flush provision for C++ client
..


Patch Set 2:

(1 comment)

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

Line 707:   data_->SetFlushMode(m);
> would it simplify things to now allow the flush mode to change if there is 
"to _not_ allow the flush mode"


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

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

[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions

2016-07-18 Thread Dan Burkert (Code Review)

Dan Burkert has posted comments on this change.

Change subject: KUDU-1311 [master] support adding and dropping range partitions
..


Patch Set 4:

(37 comments)

http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/client/client.cc
File src/kudu/client/client.cc:

PS4, Line 892: 
RETURN_NOT_OK(data_->client_->data_->WaitForAlterTableToFinish(
 : data_->client_, alter_name, deadline));
> If this fails, don't we want to clear the meta cache anyway? Maybe ClearCac
It can actually happen before waiting for the reasons outlined below.  Moved.


Line 908: // It is sufficient to clear that cache even if the table 
alteration is not
> Nit: "the cache".
Done


http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/client/client.h
File src/kudu/client/client.h:

PS4, Line 537: unless the existing range partitions are dropped first
> What happens if a single KuduTableAlterer has DropRange and AddRange on the
DropRange then AddRange is legal if the range already exists, and effectively 
truncates the existing range, but the new range is still subject to the 
visibility caveats.  AddRange then DropRange is legal if the range doesn't 
already exist, and is effectively a no-op (the tablet shouldn't even be 
created).  And finally, any combination of these is possible, e.g. Add -> Drop 
-> Add -> Drop, etc.


Line 541:   // this method returns, however other existing clients may have to 
wait for a
> Don't you mean "when Alter() returns success"?
Done


http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

Line 19: 
> What were the include and using changes for?
mistake.


http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/client/table_alterer-internal.h
File src/kudu/client/table_alterer-internal.h:

Line 57:   struct PartitioningStep {
> I think the code would be net less complex if PartitionStep and Step were c
Done


http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/integration-tests/alter_table-randomized-test.cc
File src/kudu/integration-tests/alter_table-randomized-test.cc:

Line 363: LOG(INFO) << "Adding column " << name;
> Were these LOG statements just for debugging or do you want to keep them?
They are really helpful in debugging the test, and they don't happen too often 
(insert/update/delete are common, but set to vlog).


Line 459:   scanner.SetTimeoutMillis(6);
> What's the significance of this value?
This was copied from ScanTableToStrings, which wasn't suitable anymore because 
it doesn't use a fault tolerant scanner (sort order is important).


Line 510:   vector master_addrs_;
> Unused?
Done


PS4, Line 547: } else if (r < 850 && t.num_range_partitions() < 
kMaxRangePartitions) {
 :   t.AddRangePartition();
 : } else if (r < 900) {
 :   t.DropRangePartition();
> It would be cool if this also tested "compound" operations, i.e. an AlterTa
Done


http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:

Line 27: #include "kudu/client/client.h"
> Nit: the previous sort order was more correct.
Done


Line 36: #include "kudu/master/master-test-util.h"
> Nit: likewise, this was more correct before.
Done


Line 463:   session->SetTimeoutMillis(15 * 1000);
> Why this value?
It's copied from InsertRows above.


Line 468: gscoped_ptr insert(table->NewInsert());
> Use unique_ptr here?
Done


PS4, Line 470: if (table->schema().num_columns() > 1) {
 :   RETURN_NOT_OK(insert->mutable_row()->SetInt32(1, i));
 : }
> The assumption being that every column is going to be an Int32?
Yes, this method assumes the test class's schema_ as the schema.


Line 550: int AlterTableTest::CountRows(const string& table_name) {
> Can you reuse CountTableRows() from client-test-util.h? There may be some o
Done


Line 1064: TEST_F(AlterTableTest, TestAlterRangePartitioning) {
> How about a test case for one AlterTable() adding and dropping the same par
Done


Line 1065:   gscoped_ptr table_alterer;
> unique_ptr?
Done


PS4, Line 1091:   table_alterer->wait(true);
  :   
ASSERT_OK(table_alterer->AddRangePartition(add_lower.release(), 
add_upper.release())->Alter());
> Combine?
Done


PS4, Line 1097:   ASSERT_OK(InsertRowsSequential(table_name, 0, 100));
  :   ASSERT_EQ(100, CountRows(table_name));
> This is to test that IsAlterTableInProgress()->false actually means we can 
yah, I changed this up slightly since that's not really worth testing as it was.


http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

PS4, Line 1215:   Schema schema;
  :   
RETURN_NOT_OK(SchemaFromPB(table->metadata().state().pb.schema(), ));
  :   PartitionSchema partition_schema;
  :

[kudu-CR] KUDU-1311 [master] support adding and dropping range partitions

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-1311 [master] support adding and dropping range partitions
..


Patch Set 6:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be
Gerrit-PatchSet: 6
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] [c++-client]: cache non-covering ranges in meta cache

Kudu Jenkins has posted comments on this change.

Change subject: [c++-client]: cache non-covering ranges in meta cache
..


Patch Set 9:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I05bcb3fe05d51d7c455e1d68bd2baa6f3c2b9d21
Gerrit-PatchSet: 9
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-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] async background flush provision for C++ client

2016-07-18 Thread David Ribeiro Alves (Code Review)

David Ribeiro Alves has posted comments on this change.

Change subject: async background flush provision for C++ client
..


Patch Set 2:

(62 comments)

overall I think this needs more tests. Since the flusher is a thread, likely 
the unit tests will also have to use threads to:
- make sure limits are not exceeded
- make sure that operations are in fact buffered up to a point
- Test the multiple triggers of a flush.

Beyond the the unit tests, I think this also needs an integration test that 
stresses it all.

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

Line 7: async background flush provision for C++ client
nit: capitalize (Async)


PS2, Line 7: provision
nit: remove provision


PS2, Line 10: is
nit: avoid using the passive voice. i.e. "KuduSession starts a background 
flusher thread" not a thread is started


PS2, Line 13: virtual
what's a virtual buffer?


Line 16: The flush criteria is based on buffer size for not-yet-scheduled
I'm having trouble parsing this sentence.


PS2, Line 17: operatations
nit: type


PS2, Line 20: operatations
nit: typo


PS2, Line 23: KUDU
Mention this issue in the commit message title.


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

Line 267: #ifndef NDEBUG
spurious change


Line 383: 
spurious change


Line 553:   //since the other time it's called in KuduSession::Apply()
nit: rephrase. How about:
Optimize and call write_op->SizeInBuffer() only once. We're currently calling 
it twice, here and in KuduSession::Apply()


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

Line 109:   // Return number of bytes used for batcher buffer
nit: "Returns the"
nit: period at the end os sentences/


Line 114:   // Compute in-buffer size for the given write operation
nit: Computes
nit: period


PS2, Line 216: //
no need for this comment


http://gerrit.cloudera.org:8080/#/c/3668/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

Line 1991: // return an error with session running in any supported flush mode
nit: period


PS2, Line 1993: buffer_space_bytes_limit
style: kBufferSpaceBytesLimit or BUFFER_SPACE_BYTES_LIMIT


PS2, Line 1993: static
not need for static.


PS2, Line 1994: static
same


Line 1995:   static const KuduSession::FlushMode modes[] = {
same


Line 2001:   for (const auto mode: modes) {
isn't 'auto' already const ?


Line 2007: ASSERT_TRUE(s.IsIncomplete()) << "got unexpected status: " << 
s.ToString();
nit: capitalize Got


Line 2014:   // Applying a bunch of small rows without a flush should not 
result in
this sentence is huge and hard to read. please break it down.


Line 2016:   // since there is a flow control which makes Session::Apply() 
block and wait
what: "a flow control"?


Line 2018:   {
no need for the extra { scope


Line 2023:   ASSERT_OK(ApplyInsertToSession(session.get(), client_table_, 
1, 1, "x"));
How this testing that Apply() is eventually blocking, or making sure that the 
buffer value is not surpassed for that matter?


Line 2029: // operations are put into the queue flushed after some time.
you mean : "into the queue and flushed" right?


Line 2030: // The exact timeout interval is implementation-dependent,
which timeout interval?


Line 2031: // but 100 ms is a good upper limit anyways.
s/anyways/anyway


Line 2033:   {
no need for the extra {


Line 2041: SleepFor(MonoDelta::FromMilliseconds(100));
loop instead of wait, otherwise this will be brittle in jenkins


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

Line 705:   "Cannot change flush mode when writes are buffered");
nit period


Line 707:   data_->SetFlushMode(m);
would it simplify things to now allow the flush mode to change if there is a 
flusher thread? if not would it make sense to have this stop the thread (if the 
new mode is different) and wait for it to finish before allowing to change the 
mode? seems like there is some complexity attached to the fact that we allow 
this to change at anytime. simplifying the possible states there would be good


Line 728:   return data_->SetBufferBytesLimit(size);
why not use the same method name?


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

Line 678: // The Flush() call can be used to block until the latest batch 
is sent.
maybe add/replace: "a batch is sent and the buffer has available space again"


http://gerrit.cloudera.org:8080/#/c/3668/2/src/kudu/client/session-internal.cc
File src/kudu/client/session-internal.cc:

Line 48:   // intentionally left empty
no ned for this, you can also join the brackets in the line above, i.e. {}


Line 53: WARN_NOT_OK(Stop(), "unable to stop AUTO_FLUSH_BACKGROUND thread");
capitalize. Also a WARN? what happens if this fails?


Line 58:

[kudu-CR] WIP [java-client] Re-enable multi-master tests

Kudu Jenkins has posted comments on this change.

Change subject: WIP [java-client] Re-enable multi-master tests
..


Patch Set 2: -Verified

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1051222738c84ef3d3e1a33b4981bc9454b7972
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] KUDU-1516 ksck should check for more raft-related status issues (partial)

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1516 ksck should check for more raft-related status issues 
(partial)
..


Patch Set 1:

BTW, I tried this on a cluster with a bad table:
WARNING: Unable to connect to Tablet Server 5fb7d6c7083943059521e03d6ece2863 
(10.20.132.112:7050) because Network error: Client connection negotiation 
failed: client connection to 10.20.132.112:7050: connect: Connection refused 
(error 111)
WARNING: Unable to connect to Tablet Server acd8306f95334ec1bfce8cb30d7ca36d 
(10.20.126.115:7050) because Network error: Client connection negotiation 
failed: client connection to 10.20.126.115:7050: connect: Connection refused 
(error 111)
WARNING: Unable to connect to Tablet Server dff78a5acdbb4a47ba2c7a62d1bcc5ee 
(10.20.132.107:7050) because Network error: Client connection negotiation 
failed: client connection to 10.20.132.107:7050: connect: Connection refused 
(error 111)
WARNING: Connected to 69 Tablet Servers, 3 weren't reachable
WARNING: Tablet 3bf432551c5d4c529616f8e7ce829424 of table 'usertable' does not 
have a majority of replicas in RUNNING state
WARNING: Tablet 2f652871b74b4d0f9bf99e730486a451 of table 'usertable' does not 
have a majority of replicas in RUNNING state
WARNING: Tablet b009973af71842cf99e10d25254b5557 of table 'usertable' does not 
have a majority of replicas in RUNNING state
WARNING: Tablet 71ca44eebda44903868014175e02862a of table 'usertable' does not 
have a majority of replicas in RUNNING state
WARNING: Table usertable has 4 bad tablets
INFO: Table IntegrationTestBigLinkedListHeads is HEALTHY
INFO: Table IntegrationTestBigLinkedList is HEALTHY
WARNING: 1 out of 3 tables are not in a healthy state
==
Errors:
==
Tablet server aliveness check error: Network error: Not all Tablet Servers are 
reachable
Table consistency check error: Corruption: 1 tables are bad

FAILED

>From this output you can see how it would be useful to give slightly more info 
>on why the bad tablets are bad. Let me know if you'll have time to keep 
>working on this - otherwise I might try to take it from where you left off.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec6590ba52548a9ee11d63269b134320b10809da
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] WIP [java-client] Re-enable multi-master tests

2016-07-18 Thread Jean-Daniel Cryans (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: WIP [java-client] Re-enable multi-master tests
..

WIP [java-client] Re-enable multi-master tests

This patch makes TestMasterFailover useful again. It also adds the killing of 
masters
to ITClient. Finally, it sets the raft heartbeat lower so that we don't wait 
1.5s for
leader elections.

WIP because I was getting failures before and now it just works, which makes me 
worried.
Hoping ASAN or TSAN will trigger something.

Change-Id: Ia1051222738c84ef3d3e1a33b4981bc9454b7972
---
M 
java/kudu-client/src/main/java/org/kududb/client/GetMasterRegistrationReceived.java
M java/kudu-client/src/test/java/org/kududb/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/kududb/client/ITClient.java
M java/kudu-client/src/test/java/org/kududb/client/MiniKuduCluster.java
A 
java/kudu-client/src/test/java/org/kududb/client/TestGetMasterRegistrationReceived.java
M java/kudu-client/src/test/java/org/kududb/client/TestMasterFailover.java
6 files changed, 243 insertions(+), 28 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia1051222738c84ef3d3e1a33b4981bc9454b7972
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] WIP [java-client] Re-enable multi-master tests

Kudu Jenkins has posted comments on this change.

Change subject: WIP [java-client] Re-enable multi-master tests
..


Patch Set 2:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1051222738c84ef3d3e1a33b4981bc9454b7972
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] KUDU-1374: send full tablet report when new leader master is detected

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1374: send full tablet report when new leader master is 
detected
..


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3643/5//COMMIT_MSG
Commit Message:

Line 13: there was a way to mock a server-side krpc component as easily as it 
is to
what about injecting a master crash in the AsyncSendFoo() method? that would be 
likely to trigger this in many cases, right? (not a guarantee that the response 
was quicker than the starting of the async stuff, but pretty likely I think?)


Line 15: multi-master tests, though.
is there a test you can loop before/after to verify this?


http://gerrit.cloudera.org:8080/#/c/3643/5/src/kudu/tserver/heartbeater.cc
File src/kudu/tserver/heartbeater.cc:

Line 175:   // Indicates that the thread should set a full tablet report. Set 
when
s/set/send/


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic16fc46736476dba39616e79ecfe79eee48b3d7f
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1358 (part 3): new multi-master stress test

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1358 (part 3): new multi-master stress test
..


Patch Set 9:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3611/9/src/kudu/integration-tests/master-stress-test.cc
File src/kudu/integration-tests/master-stress-test.cc:

PS9, Line 73: tT
admin?


Line 219: // TODO: Should be fixed with Exactly Once semantics.
would be nice to tie these to JIRAs


Line 223:   num_altered++;
why not get rid of these local variables and just IncrementBy(1) on the global 
atomic?


Line 323:   SleepFor(MonoDelta::FromMilliseconds(100));
would be good to randomize the sleeps, so you might catch some slightly 
different interleavings


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I40b5b78c100a7b427b2f4aac3a54665e82a9618c
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] async background flush provision for C++ client

Kudu Jenkins has posted comments on this change.

Change subject: async background flush provision for C++ client
..


Patch Set 2:

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

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

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

[kudu-CR] KUDU-1358 (part 2): heartbeat to every master

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1358 (part 2): heartbeat to every master
..


Patch Set 9:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3610/9/src/kudu/tserver/heartbeater.cc
File src/kudu/tserver/heartbeater.cc:

Line 164:   mutable std::atomic_int next_report_seq_;
hrm, this really has to be mutable?


Line 522: TabletReportState state;
maybe use = { seqno }; here? that way you'll get a warning at some point later 
if you added a field?


PS9, Line 528: const 
maybe makes more sense for this to not be 'const' since it changes the sequence 
number (rather than making it mutable). willing to accept this way too though


http://gerrit.cloudera.org:8080/#/c/3610/9/src/kudu/tserver/heartbeater.h
File src/kudu/tserver/heartbeater.h:

Line 57:   // not dirty once the report has been acknowledged by every master.
this makes it sound like it will keep reporting as "dirty" to all masters until 
all masters have acknowledged. Really, dirtiness is tracked separately per 
master, right?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic85ac4193462d21c989dbd7874b451e8eaab8e3e
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR](gh-pages) Add dropdown menu for Community nav button

Mike Percy has uploaded a new patch set (#3).

Change subject: Add dropdown menu for Community nav button
..

Add dropdown menu for Community nav button

* The dropdown is disabled on small screens
* Also remove justified nav CSS file, since it's no longer used
* Also remove "Fork me on GitHub" ribbon

Change-Id: I8c0f2671e2257a4249b4b26d1cc8513ee0e47362
---
M _includes/top_common.html
D css/justified-nav.css
M css/kudu.css
3 files changed, 143 insertions(+), 156 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8c0f2671e2257a4249b4b26d1cc8513ee0e47362
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR](gh-pages) Add dropdown menu for Community nav button

Mike Percy has posted comments on this change.

Change subject: Add dropdown menu for Community nav button
..


Patch Set 2:

(2 comments)

> One more parting thought: might be worth a 
 > here advising people that they shouldn't add any content that is
 > _only_ linked from this dropdown, since the dropdown doesn't show
 > up on mobile.

Done

http://gerrit.cloudera.org:8080/#/c/3665/2/_includes/top_common.html
File _includes/top_common.html:

PS2, Line 74: http://localhost:4000/
> woops
Thanks for the catch


http://gerrit.cloudera.org:8080/#/c/3665/2/css/kudu.css
File css/kudu.css:

Line 125: @media (max-width:870px) {
> it's sort of weird that the dropdown disappears at a different width than t
Changed to max-width:768px (instead of 767px) because I don't think it works 
quite right on iPad yet, and that at least covers vertical iPad. Horizontal 
iPad will see the menu but a click ends up flickering the menu, then going to 
community.html ... but it will take some messing around to deal with that case 
I think since it appears to detect at 1024px


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c0f2671e2257a4249b4b26d1cc8513ee0e47362
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] async background flush provision for C++ client

Alexey Serbin has posted comments on this change.

Change subject: async background flush provision for C++ client
..


Patch Set 1:

> What was the rationale behind using a dedicated thread to manage
 > background flushing? I think synchronization would be net simpler
 > if instead, we reused the messenger's reactor threads for
 > background flushing. They're already used to run callbacks whenever
 > a Proxy::FooAsync() call is made, and you can use 
 > Messenger::ScheduleOnReactor()
 > to schedule the background flushing to happen at some time in the
 > future.
 > 
 > Some other arguments in favor of using reactor threads:
 > 1. It reduces the thread count  of the client.
 > 2. Structurally it makes the implementation similar to the Java
 > client.

I thought messenger reactor threads are for messaging, and I was not so 
familiar with that part of code.  OK, I'll explore that option in this context.

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

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

[kudu-CR](gh-pages) Clean up community web page and add commits@ mailing list

Todd Lipcon has posted comments on this change.

Change subject: Clean up community web page and add commits@ mailing list
..


Patch Set 1: Code-Review+2

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

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

[kudu-CR](gh-pages) Add dropdown menu for Community nav button

Todd Lipcon has posted comments on this change.

Change subject: Add dropdown menu for Community nav button
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3665/2/_includes/top_common.html
File _includes/top_common.html:

PS2, Line 74: http://localhost:4000/
woops


http://gerrit.cloudera.org:8080/#/c/3665/2/css/kudu.css
File css/kudu.css:

Line 125: @media (max-width:870px) {
it's sort of weird that the dropdown disappears at a different width than the 
style switches to the 'hamburger' menu. ie if I just have a not-wide screen, I 
don't get the dropdown, but I still have the horizontal menu.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c0f2671e2257a4249b4b26d1cc8513ee0e47362
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Add time/watermark based garbage collection to ResultTracker

Kudu Jenkins has posted comments on this change.

Change subject: WIP: Add time/watermark based garbage collection to 
ResultTracker
..


Patch Set 12: -Verified

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

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

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

[kudu-CR] master: do not delete unknown tablets

Todd Lipcon has posted comments on this change.

Change subject: master: do not delete unknown tablets
..


Patch Set 5:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/3645/5//COMMIT_MSG
Commit Message:

Line 7: master: do not delete unknown tablets
I wonder if we now need some tool to "reformat" a tablet server? or to insert a 
dummy "deleted table" or "deleted tablets" entry into the master? i.e what if 
for some reason we do end up with some unknown tablets that are being reported, 
and we want to delete them to reclaim the space. Right now we'd have to send 
manual delete_tablet RPCs to every server which sounds kind of complicated to 
script up.

Another option would be some kind of "automatically resurrect the tablet based 
on reports" or something?

(thinking about the case here where we have taken a backup of a master and roll 
back in time, for example).

(of course feel free to backlog this, just curious if we anticipate a support 
burden here)


Line 9: Quoting from the multi-master design doc:
nit: can you provide a link or source code path here?


http://gerrit.cloudera.org:8080/#/c/3645/5/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

PS5, Line 1546: INFO)
WARNING might be more appropriate here? or perhaps INFO if it's an incremental 
report, and WARNING if it's a full report? (since the latter indicates a 
persistent condition whereas the former might be a transient issue as in the 
race described above?)


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

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

[kudu-CR] async background flush provision for C++ client

Adar Dembo has posted comments on this change.

Change subject: async background flush provision for C++ client
..


Patch Set 1:

What was the rationale behind using a dedicated thread to manage background 
flushing? I think synchronization would be net simpler if instead, we reused 
the messenger's reactor threads for background flushing. They're already used 
to run callbacks whenever a Proxy::FooAsync() call is made, and you can use 
Messenger::ScheduleOnReactor() to schedule the background flushing to happen at 
some time in the future.

Some other arguments in favor of using reactor threads:
1. It reduces the thread count  of the client.
2. Structurally it makes the implementation similar to the Java client.

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

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

[kudu-CR](gh-pages) Add weekly update for 7/18

Todd Lipcon has posted comments on this change.

Change subject: Add weekly update for 7/18
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib0ee48c282377263dace0ac66fe3043aaf856c47
Gerrit-PatchSet: 1
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) Update the docs webpages to reflect the master branch

Todd Lipcon has posted comments on this change.

Change subject: Update the docs webpages to reflect the master branch
..


Patch Set 1:

The reason they're out of date is that we typically wait for a release to 
publish the new docs. If we want to start publishing master ("trunk") docs, I 
think we should make it clear in the URL (and maybe a header on the pages) 
saying something like "This page refers to an unreleased development version of 
Apache Kudu." with some kind of link back to the latest release.

Otherwise the average user (who does not build and run trunk) will be confused 
when they see APIs or features that aren't available in what they're running. 
For example, check out what LLVM does with the big red WARNING: 
http://llvm.org/docs/ReleaseNotes.html

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5a760ec169880954961f2da2a288f4d963e87d84
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR](gh-pages) Update the docs webpages to reflect the master branch

Mike Percy has posted comments on this change.

Change subject: Update the docs webpages to reflect the master branch
..


Patch Set 1:

(4 comments)

We should fix some "wrong docs" before regenerating this and pushing it

Specifically we are doing 0.10.0 instead of 1.0.0 and also 0.10.0 is not 
released yet, which should be made clear in the docs

http://gerrit.cloudera.org:8080/#/c/3672/1/docs/release_notes.html
File docs/release_notes.html:

Line 85: Release notes 
specific to 1.0.0
We still need to change 1.0.0 here and below to 0.10.0, also it should say 
something like (unreleased) I think. Maybe JD has thoughts on this


Line 288: http://getkudu.io/2016/04/26/ycsb.html;>Experiments using 
YCSB indicate that these
Undesirable change


Line 964: http://getkudu.io;>Kudu Website
Undesirable change


Line 1030: Release notes specific to 1.0.0
says 1.0.0


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5a760ec169880954961f2da2a288f4d963e87d84
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Add time/watermark based garbage collection to ResultTracker

Kudu Jenkins has posted comments on this change.

Change subject: WIP: Add time/watermark based garbage collection to 
ResultTracker
..


Patch Set 12:

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

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

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

[kudu-CR] WIP: Add time/watermark based garbage collection to ResultTracker

2016-07-18 Thread David Ribeiro Alves (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: Add time/watermark based garbage collection to 
ResultTracker
..

WIP: Add time/watermark based garbage collection to ResultTracker

This adds time and watermark based garbage collection to the ResultTracker.
Regarding time GC, there are two ttl's, a client ttl and a response ttl.

After the response ttl has elapsed, we garbage collect responses
but the ResultTracker remembers that it doesn't know them, so if
the client retries a request older than that it gets a meaningful
error back, stating that the request is stale.

After the client ttl period without hearing back from a client,
we gc the client state entirely, meaning all requests from that
client will be treated as new.

Regarding watermark GC the algorithm is simple, we trust the client
to tell us what's its lowest incomplete sequence number and we gc
everything below that.

This adds a simple test that makes sure this basically
works, but is still missing a proper integration test.

Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
9 files changed, 248 insertions(+), 36 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR](gh-pages) Update the docs webpages to reflect the master branch

2016-07-18 Thread Dinesh Bhat (Code Review)

Hello Jean-Daniel Cryans, Mike Percy,

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

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

to review the following change.

Change subject: Update the docs webpages to reflect the master branch
..

Update the docs webpages to reflect the master branch

Change-Id: I5a760ec169880954961f2da2a288f4d963e87d84
---
M docs/administration.html
M docs/configuration.html
M docs/configuration_reference.html
M docs/configuration_reference_unsupported.html
M docs/contributing.html
M docs/developing.html
M docs/index.html
M docs/installation.html
M docs/kudu-master_configuration_reference.html
M docs/kudu-master_configuration_reference_unsupported.html
M docs/kudu-tserver_configuration_reference.html
M docs/kudu-tserver_configuration_reference_unsupported.html
M docs/kudu_impala_integration.html
M docs/quickstart.html
M docs/release_notes.html
M docs/schema_design.html
M docs/style_guide.html
M docs/transaction_semantics.html
M docs/troubleshooting.html
19 files changed, 794 insertions(+), 303 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5a760ec169880954961f2da2a288f4d963e87d84
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Mike Percy

[kudu-CR] KUDU-1358 (part 1): master should accept heartbeat even if follower

Todd Lipcon has posted comments on this change.

Change subject: KUDU-1358 (part 1): master should accept heartbeat even if 
follower
..


Patch Set 9:

(1 comment)

Any way to system test this, like calling ListTabletServers against a follower 
master? (or visiting its /tablet-servers web page?)

http://gerrit.cloudera.org:8080/#/c/3609/9/src/kudu/master/catalog_manager.h
File src/kudu/master/catalog_manager.h:

PS9, Line 366:   };
DISALLOW_COPY_AND_ASSIGN


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I578674927b65b4171e8437de8515130e4a0ed139
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] doxygen for C++ client API

Adar Dembo has posted comments on this change.

Change subject: doxygen for C++ client API
..


Patch Set 9:

(65 comments)

http://gerrit.cloudera.org:8080/#/c/3619/9/src/kudu/client/client.h
File src/kudu/client/client.h:

High level formatting questions:
1. I noticed that you've reformatted the comments to insert two spaces when 
beginning a new sentence rather than one. There's no standard for this as far 
as Kudu is concerned, but I think most of us only use one space. Is this change 
purely stylistic, or is it mandated by doxygen?
2. Should the sentence summarizing a given method end with a period? In some 
cases (e.g. SetVerboseLogLevel, UninstallLoggingCallback) it does, while in 
others (e.g. SetInternalSignalNumber, GetShortVersionString) it doesn't. Is 
there a protocol here? Or is it just inconsistent? If it's just an 
inconsistency, could you fix it? The same applies for parameters and return 
values.
3. Some getters only have a @return while others have both a @return and a 
short summarizing sentence. Why the inconsistency? If either are acceptable, 
I'd prefer if simple getters just had a @return, as it's less noise.


PS9, Line 91: Before a callback is registered, all internal client log events
: /// are logged to the stderr.
Nit: why combine this sentence into the previous paragraph instead of leaving 
it on its own as before?


Line 131: ///   Signal number to use for internal
Nit: for internal what? Maybe you meant "Signal number to use internally"?


PS9, Line 222: Each KuduClient instance is sandboxed with no
 : /// global cross-client state.
Not related to your change, but this isn't strictly true anymore. Basically, 
free functions (like those at the top of client.h) affect all clients because 
they modify global state.


Line 247:   /// @return Pointer to newly created instance: it's the caller's
Nit: semi-colon is more appropriate here.


Line 277:   /// Check if table alteration is in process
Nit: in-progress, to be consistent with IsCreateTableInProgress.


PS9, Line 282:   ///   Set to @c true if an AlterTable operation is in-progress;
 :   ///   set to @c false otherwise
Nit: can you reword to be consistent with IsCreateTableInProgress, since they 
both work the same way?


Line 287:   /// Get scheme for a table
Nit: schema, not scheme.


Line 290:   ///   Name of the table if question
Nit: in question


Line 292:   ///   Raw pointer to the schema object: caller gets ownership
Nit: semi-colon.


Line 310:   ///   Substring-filter to use; empty sub-string filter matches any 
tables
Nit: Substring filter


Line 320:   ///   Set only on success: set to @c true iff table exists
Nit: semi-colon


PS9, Line 326:   /// If the table has not been opened before, then open the 
table
 :   /// with the given name.  If the table has not been opened 
before
 :   /// for this client, this will do an RPC to ensure that the 
table exists
 :   /// and look up its schema.
Also not related to this change, but the RPC and schema lookup are always done, 
regardless if "the table has not been opened before" or not.


Line 346:   /// @return A new session object: caller is responsible for 
destroying it.
Nit: semi-colon.


Line 359:   /// @return @c true iff it's a multi-master session
This isn't related to sessions per-se, it's true if the client was configured 
to talk to multiple Kudu masters.


Line 365:   /// @return Default timeout for RPC operations.
Nit: we do distinguish between "operations" and "RPCs". The former are logical 
actions, like "create this new table and wait until it's fully created", while 
the latter are single messages sent to a server, like "Write() sent to a tablet 
server". The distinction is important because an operation may require sending 
multiple RPCs, or retrying some RPCs.

As such, "RPC operations" is confusing. Should probably just drop "operations".


Line 376:   /// by this client.  Check KuduScanner for more details on 
timestamps.
Nit: why the change from "See KuduScanner" to "Check KuduScanner"? Was the 
former unclear or imprecise?


PS9, Line 385: To use this the user must obtain
 :   /// the HybridTime encoded timestamp from the first client with
 :   /// KuduClient::GetLatestObservedTimestamp() and then set it
 :   /// in the new client with this method.
Nit: let's separate this into a new paragraph.


Line 432: class KUDU_EXPORT KuduTableCreator {
The "Required" and "Optional" tags are gone. I think they were useful; can we 
restore them?


Line 438:   /// This method must be called at an object before .
This sentence is unclear, and there's an extra space at the end.


Line 447:   /// Set the schema with which to create next table.
Nit: the subsequent builder methods refer to the table to be created as simply 
"the table". Can we do that here too, instead of "next table"?

[kudu-CR](gh-pages) Add weekly update for 7/18

2016-07-18 Thread Jean-Daniel Cryans (Code Review)

Jean-Daniel Cryans has posted comments on this change.

Change subject: Add weekly update for 7/18
..


Patch Set 1: Code-Review+2

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

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

[kudu-CR] [java client] Redo how we manage exceptions

2016-07-18 Thread Jean-Daniel Cryans (Code Review)

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java client] Redo how we manage exceptions
..


Patch Set 8:

> hm, what's the status on this. Still waiting on Dan?

Yeah I told Dan that helping out Adar first with reviews was better since he's 
basing his add/remove partitions on top of Adar's patches.

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

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

[kudu-CR] Fix flaky disk reservation-itest

Todd Lipcon has posted comments on this change.

Change subject: Fix flaky disk_reservation-itest
..


Patch Set 1: Code-Review+2

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

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

[kudu-CR] Fix flaky disk reservation-itest

Todd Lipcon has submitted this change and it was merged.

Change subject: Fix flaky disk_reservation-itest
..


Fix flaky disk_reservation-itest

There are two fixes in this patch for two separate types of failures
seen on Jenkins for this test:

1. Fix a data race in DiskReservationITest.TestFillMultipleDisks

We can't override gflag strings at runtime in a thread-safe manner,
although this test was attempting to.

Take what used to be a single parsed string gflag and replace it with 2
path strings and 2 integer overrides, one for each path. That makes 4
new test-only gflags total. Only the integer flags are modified at
runtime.

2. Fix a startup race between the TestWorkload client thread and
   SetFlags() in DiskReservationITest.TestWalWriteToFullDiskAborts

We need to wait for some rows to be written after starting up the
TestWorkload threads in TestWalWriteToFullDiskAborts before we allow the
TS to crash by setting gflags. If we don't, the test gets confused
because the TestWorkload client thread may not be able to resolve where
the tablet is located. The previous failures were because we sometimes
managed to crash the TS before it sent its tablet report to the master.

After applying these changes, I looped disk_reservation-itest 1000x in
TSAN mode and got no failures.

Change-Id: Ica86390b49d459e079807d777e97c47fa35134d1
Reviewed-on: http://gerrit.cloudera.org:8080/3652
Tested-by: Mike Percy 
Reviewed-by: Todd Lipcon 
---
M src/kudu/integration-tests/disk_reservation-itest.cc
M src/kudu/util/env_util.cc
2 files changed, 61 insertions(+), 29 deletions(-)

Approvals:
  Mike Percy: Verified
  Todd Lipcon: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ica86390b49d459e079807d777e97c47fa35134d1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR](gh-pages) Add weekly update for 7/18

Hello Jean-Daniel Cryans,

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

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

to review the following change.

Change subject: Add weekly update for 7/18
..

Add weekly update for 7/18

Change-Id: Ib0ee48c282377263dace0ac66fe3043aaf856c47
---
A _posts/2016-07-18-weekly-update.md
1 file changed, 59 insertions(+), 0 deletions(-)


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

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

[kudu-CR] master: add assert checks for leader lock

Adar Dembo has submitted this change and it was merged.

Change subject: master: add assert checks for leader_lock
..


master: add assert checks for leader_lock

A side effect of recursive checking in RWMutex is that we can now assert
that a RWMutex is held for reading/writing. Let's add that to the various
catalog manager entry points.

Change-Id: Iefb5762c70192b27490cc71e20568815d18d6ad5
Reviewed-on: http://gerrit.cloudera.org:8080/3642
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert 
---
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/integration-tests/mini_cluster.cc
M src/kudu/integration-tests/mini_cluster.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test-util.h
M src/kudu/master/master.cc
9 files changed, 78 insertions(+), 41 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Iefb5762c70192b27490cc71e20568815d18d6ad5
Gerrit-PatchSet: 6
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] rw mutex: prevent recursive use

Adar Dembo has submitted this change and it was merged.

Change subject: rw_mutex: prevent recursive use
..

rw_mutex: prevent recursive use

Todd provided an example[1] of deadlocked rwlocks due to a fairness policy.
In the example, T1 (holding the lock for reading) join()ed on T2 (trying to
acquire the lock for reading) all while T3 was trying to acquire the lock
for writing. The lock's fairness policy prevented T2 from acquiring the read
lock thus deadlocking all three threads. The takeaway is to be careful when
calling join() while holding locks.

Beyond that, deadlocks can arise if rwlocks are taken recursively. That's
not a feature we need in our rwlocks, so I tried to disable it at the
pthread level. Unfortunately, the best I can do is disable recursive write
lock acquisition; read locks are apparently always recursive (see "man
pthread_rwlockattr_setkind_np"). So instead, I built recursive checking into
the RWMutex itself. It's quite slow so it's only present in debug builds.

Note that pthread rwlocks do have some built-in deadlock detection (i.e.
lock calls may return EDEADLK), but it doesn't appear to be comprehensive.

1.
https://issues.apache.org/jira/browse/HDFS-2223?focusedCommentId=13097647=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13097647

Change-Id: I7ae30ec123a16c39ef0c15ee2d2176f807df03db
Reviewed-on: http://gerrit.cloudera.org:8080/3641
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert
Reviewed-by: Todd Lipcon
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/rw_mutex-test.cc
M src/kudu/util/rw_mutex.cc
M src/kudu/util/rw_mutex.h
4 files changed, 314 insertions(+), 4 deletions(-)

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

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

Gerrit-MessageType: merged
Gerrit-Change-Id: I7ae30ec123a16c39ef0c15ee2d2176f807df03db
Gerrit-PatchSet: 7
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] doxygen for C++ client API

Mike Percy has posted comments on this change.

Change subject: doxygen for C++ client API
..


Patch Set 9:

One thing we should add to this (it can be a follow-up commit).

It needs to be integrated into make_site.sh and the web site itself.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] KUDU-1492: Show column encodings/compression on table page in master Test results are in JIRA: https://issues.apache.org/jira/browse/KUDU-1492

Alexey Serbin has posted comments on this change.

Change subject: KUDU-1492: Show column encodings/compression on table page in 
master Test results are in JIRA: https://issues.apache.org/jira/browse/KUDU-1492
..


Patch Set 1: Code-Review+1

(2 comments)

LGTM, just a couple questions.

http://gerrit.cloudera.org:8080/#/c/3667/1/src/kudu/server/webui_util.cc
File src/kudu/server/webui_util.cc:

Line 35:std::stringstream* output) {
This is not part of this change, but just an additional comment.  Does it make 
sense to change type of output into more narrower type ostringstream since the 
method is supposed only to output some data in there?


Line 56: *output << 
Substitute("$0$1$2$3"
I, as a reader, would appreciate some text example of the output in the 
comment.  Would it make sense?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I990a8d790ab71a05be04f0b7468b5da0894478e8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] Memory tracking for result tracker

Adar Dembo has posted comments on this change.

Change subject: Memory tracking for result tracker
..


Patch Set 18:

(1 comment)

I don't have any more comments except on the tests.

http://gerrit.cloudera.org:8080/#/c/3627/18/src/kudu/rpc/rpc-stress-test.cc
File src/kudu/rpc/rpc-stress-test.cc:

Line 260: SleepFor(MonoDelta::FromMilliseconds(100));
I'm concerned that the sleeps will make these tests flaky. Could you loop them 
a thousand times in TSAN mode?


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

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

[kudu-CR] async background flush provision for C++ client

Alexey Serbin has uploaded a new change for review.

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

Change subject: async background flush provision for C++ client
..

async background flush provision for C++ client

Implemented AUTO_FLUSH_BACKGROUND for C++ client library.
A dedicated background flusher thread is started by KuduSession
instance for this purpose.  In AUTO_FLUSH_BACKGROUND mode the
KuduSession::Apply() method blocks if there is not enough place
in the virtual buffer.  The limit on the buffer size is set by
KuduSession::SetMutationBufferSpace().

The flush criteria is based on buffer size for not-yet-scheduled
session's write operatations and timeout.

The flow control criteria in KuduSession::Apply() is based on
total buffer size of session's write operatations.

Addressed the following JIRA issues:
  KUDU-456 Implement AUTO_FLUSH_BACKGROUND flush mode
  KUDU-1376 KuduSession::SetMutationBufferSpace is not defined

Change-Id: Idfde05bef342db24990c6e3da3b0270c3bb37a9d
---
M src/kudu/client/batcher.cc
M src/kudu/client/batcher.h
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/error_collector.cc
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
7 files changed, 432 insertions(+), 103 deletions(-)


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

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

[kudu-CR] KUDU-1492: Show column encodings/compression on table page in master Test results are in JIRA: https://issues.apache.org/jira/browse/KUDU-1492

Mike Percy has posted comments on this change.

Change subject: KUDU-1492: Show column encodings/compression on table page in 
master Test results are in JIRA: https://issues.apache.org/jira/browse/KUDU-1492
..


Patch Set 1:

(2 comments)

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

Line 7: KUDU-1492: Show column encodings/compression on table page in master
> leave a blank line between the commit title and the description.
This is a good summary of best practices for this: 
http://chris.beams.io/posts/git-commit/#seven-rules


http://gerrit.cloudera.org:8080/#/c/3667/1/src/kudu/server/webui_util.cc
File src/kudu/server/webui_util.cc:

Line 39:   << "Encoding/Compression"
How about we use two different columns for each of encoding and compression?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I990a8d790ab71a05be04f0b7468b5da0894478e8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Add time/watermark based garbage collection to ResultTracker

Kudu Jenkins has posted comments on this change.

Change subject: WIP: Add time/watermark based garbage collection to 
ResultTracker
..


Patch Set 11:

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

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

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

[kudu-CR] WIP: Add time/watermark based garbage collection to ResultTracker

2016-07-18 Thread David Ribeiro Alves (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: Add time/watermark based garbage collection to 
ResultTracker
..

WIP: Add time/watermark based garbage collection to ResultTracker

This adds time and watermark based garbage collection to the ResultTracker.
Regarding time GC, there are two ttl's, a client ttl and a response ttl.

After the response ttl has elapsed, we garbage collect responses
but the ResultTracker remembers that it doesn't know them, so if
the client retries a request older than that it gets a meaningful
error back, stating that the request is stale.

After the client ttl period without hearing back from a client,
we gc the client state entirely, meaning all requests from that
client will be treated as new.

Regarding watermark GC the algorithm is simple, we trust the client
to tell us what's its lowest incomplete sequence number and we gc
everything below that.

This adds a simple test that makes sure this basically
works, but is still missing a proper integration test.

Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
9 files changed, 250 insertions(+), 36 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] WIP: Add time/watermark based garbage collection to ResultTracker

Kudu Jenkins has posted comments on this change.

Change subject: WIP: Add time/watermark based garbage collection to 
ResultTracker
..


Patch Set 10:

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

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

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

[kudu-CR] WIP: Add time/watermark based garbage collection to ResultTracker

2016-07-18 Thread David Ribeiro Alves (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: WIP: Add time/watermark based garbage collection to 
ResultTracker
..

WIP: Add time/watermark based garbage collection to ResultTracker

This adds time and watermark based garbage collection to the ResultTracker.
Regarding time GC, there are two ttl's, a client ttl and a response ttl.

After the response ttl has elapsed, we garbage collect responses
but the ResultTracker remembers that it doesn't know them, so if
the client retries a request older than that it gets a meaningful
error back, stating that the request is stale.

After the client ttl period without hearing back from a client,
we gc the client state entirely, meaning all requests from that
client will be treated as new.

Regarding watermark GC the algorithm is simple, we trust the client
to tell us what's its lowest incomplete sequence number and we gc
everything below that.

This adds a simple test that makes sure this basically
works, but is still missing a proper integration test.

Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
---
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/service_if.cc
M src/kudu/tablet/tablet_bootstrap.cc
M src/kudu/tablet/transactions/transaction_driver.cc
9 files changed, 249 insertions(+), 36 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c8e7b7191ca14842a31b64813ed498bdf626fa8
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] Make block manager-test work on systems without hole-punching

2016-07-18 Thread Will Berkeley (Code Review)

Will Berkeley has posted comments on this change.

Change subject: Make block_manager-test work on systems without hole-punching
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3636/2/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:

Line 85: template 
> How about just add a RETURN_NOT_LOG_BLOCK_MANAGER() here at the top of SetU
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56af74ad6b973c90057680be719b257109924fca
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] Make block manager-test work on systems without hole-punching

2016-07-18 Thread Will Berkeley (Code Review)

Hello Kudu Jenkins,

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

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

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

Change subject: Make block_manager-test work on systems without hole-punching
..

Make block_manager-test work on systems without hole-punching

Commit 47c023f5eba708a184666e53d4a3000177a32fbc changed
block_manager-test so that it used a macro to return from tests that
weren't using the log block manager. This macro also needs to be
called in the Setup() method for BlockManagerTest.

Change-Id: I56af74ad6b973c90057680be719b257109924fca
---
M src/kudu/fs/block_manager-test.cc
1 file changed, 7 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I56af74ad6b973c90057680be719b257109924fca
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy

[kudu-CR] Make block manager-test work on systems without hole-punching

Kudu Jenkins has posted comments on this change.

Change subject: Make block_manager-test work on systems without hole-punching
..


Patch Set 3:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I56af74ad6b973c90057680be719b257109924fca
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] KUDU-1492: Show column encodings/compression on table page in master Test results are in JIRA: https://issues.apache.org/jira/browse/KUDU-1492

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-1492: Show column encodings/compression on table page in 
master Test results are in JIRA: https://issues.apache.org/jira/browse/KUDU-1492
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I990a8d790ab71a05be04f0b7468b5da0894478e8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] KUDU-1492: Show column encodings/compression on table page in master Test results are in JIRA: https://issues.apache.org/jira/browse/KUDU-1492

2016-07-18 Thread Dinesh Bhat (Code Review)

Hello Jean-Daniel Cryans, Mike Percy, Adar Dembo,

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

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

to review the following change.

Change subject: KUDU-1492: Show column encodings/compression on table page in 
master Test results are in JIRA: https://issues.apache.org/jira/browse/KUDU-1492
..

KUDU-1492: Show column encodings/compression on table page in master
Test results are in JIRA: https://issues.apache.org/jira/browse/KUDU-1492

Change-Id: I990a8d790ab71a05be04f0b7468b5da0894478e8
---
M src/kudu/server/webui_util.cc
1 file changed, 8 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I990a8d790ab71a05be04f0b7468b5da0894478e8
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Mike Percy

[kudu-CR](gh-pages) Add dropdown menu for Community nav button

Mike Percy has uploaded a new patch set (#2).

Change subject: Add dropdown menu for Community nav button
..

Add dropdown menu for Community nav button

* The dropdown is disabled on small screens
* Also remove justified nav CSS file, since it's no longer used
* Also remove "Fork me on GitHub" ribbon

Change-Id: I8c0f2671e2257a4249b4b26d1cc8513ee0e47362
---
M _includes/top_common.html
D css/justified-nav.css
M css/kudu.css
3 files changed, 141 insertions(+), 156 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8c0f2671e2257a4249b4b26d1cc8513ee0e47362
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR](gh-pages) Clean up community web page and add commits@ mailing list

Mike Percy has posted comments on this change.

Change subject: Clean up community web page and add commits@ mailing list
..


Patch Set 1:

Rendered HTML at: http://mpercy.github.io/kudu/

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

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

[kudu-CR](gh-pages) Add dropdown menu for Community nav button