date:20160714

Jean-Daniel Cryans has posted comments on this change.

Change subject: WIP: Add garbage collection to ResultTracker
..


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3628/5/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:

PS5, Line 45:  
> not sure that's a rule. but Done
Looking almost anywhere else it's done with trailing whitespaces.


PS5, Line 91: // If the arriving request is older than our per-client GC 
watermark, report its
:   // staleness to the client.
> you mean fatal in the sense that the client can't recover? yeah (not in the
Yeah that's what I meant. So is this coming back as a row error or an RPC error?


-- 
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: 5
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: Yes

[kudu-CR] Integrate the result tracker with writes

Jean-Daniel Cryans has posted comments on this change.

Change subject: Integrate the result tracker with writes
..


Patch Set 28: Code-Review+2

-- 
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: 28
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: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Move the maintenance manager to util

Jean-Daniel Cryans has posted comments on this change.

Change subject: Move the maintenance manager to util
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcf072d443ac3d069bda15b9dc0f8c442b9ac5c0
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: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
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 7:

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

-- 
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: 7
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] [c++-client]: cache non-covering ranges in meta cache

Hello Kudu Jenkins,

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

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

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

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

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

This commit introduces a few features to the meta cache, all with the aim of
making it compatible with the upcoming add/drop range partitions feature.

1) Non-covered range partitions are now cached in the meta cache.  This is
achieved by storing MetaCacheEntry objects in the meta cache's partition-key
index instead of RemoteTablets.  The MetaCacheEntry holds either a RemoteTablet,
in which case it represents a covered partition range, or it represents a
non-covered partition range.

2) Entries are now removed from the meta cache's partition-key index when it can
be determined that the entries are no longer valid from the results of a
GetTableLocations RPC.

3) A basic TTL has been added to the GetTableLocationsResponsePB so that the
client can properly refresh the meta cache when necessary. The TTL is
configurable by the master, and defaults to one hour.

Change-Id: I05bcb3fe05d51d7c455e1d68bd2baa6f3c2b9d21
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/schema.h
M src/kudu/client/table-internal.cc
M src/kudu/gutil/map-util.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/tools/ksck_remote.cc
10 files changed, 421 insertions(+), 122 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I05bcb3fe05d51d7c455e1d68bd2baa6f3c2b9d21
Gerrit-PatchSet: 7
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

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

Hello Kudu Jenkins,

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

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

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

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

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

Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/table_alterer-internal.cc
M src/kudu/client/table_alterer-internal.h
M src/kudu/common/partition.cc
M src/kudu/integration-tests/alter_table-randomized-test.cc
M src/kudu/integration-tests/alter_table-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tools/ksck.cc
17 files changed, 713 insertions(+), 119 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins

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

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

-- 
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: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
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

Dan Burkert has posted comments on this change.

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


Patch Set 7:

(13 comments)

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

PS5, Line 1223:   // Clear the cache.
  :   meta_cache->ClearCacheForTesting();
  :   internal::MetaCacheEntry entry;
  :   
ASSERT_FALSE(meta_cache->LookupTabletByKeyFastPath(client_table_.get(), "", 
));
  : 
> You can instantiate a FlagSaver instead.
Done


http://gerrit.cloudera.org:8080/#/c/3581/5/src/kudu/client/client.h
File src/kudu/client/client.h:

Line 300:   FRIEND_TEST(ClientTest, TestScanTimeout);
> Nit: sort alphabetically.
Done


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

PS5, Line 753:   if (new_status.IsServiceUnavailable()) {
 : // One or more of the tablets is not running; retry after a 
backoff period.
 : mutable_retrier()->DelayedRetry(this, new_status);
 : ignore_result(delete_me.release());
 : return;
 :   }
> To be clear, this has nothing to do with non-covering range partition suppo
Yes, that's correct.  This manifested in some of the more aggressive add/drop 
partitions tests that come in the next patch, but I thought it was more 
appropriate to fix in this patch.


Line 790:   const auto& tablet_locations = rpc.resp().tablet_locations();
> rpc.resp().tablet_locations() is called four times, maybe pull it out into 
Done


PS5, Line 817: lthough the existence of A as an initial
> Can you pull this (and end) into local variables?
Done


PS5, Line 848:  entry(expiration_time, last_upper_bound, tablet_lower_bound);
> This assumes that a tablet can't change in partition size (i.e. that should
We discussed this off-line and decided that the CHECKs are fine for now.  A 
malformed master response shouldn't result in memory corruption, and having a 
CHECK is good from a debugging standpoint.


PS5, Line 865:   scoped_refptr remote = 
FindPtrOrNull(tablets_by_id_, tablet_id);
 :   if (remote.get() != nullptr) {
 : // Partition should not have changed.
 : DCHECK_EQ(tablet_lower_bound, 
remote->partition().partition_key_start());
 : DCHECK_EQ(tablet_upper_bound, 
remote->partition().partition_key_end());
> Maybe encapsulate this (and the non-covered range variant) in a helper meth
I'm a little hesitant to do that because the cases are so short (just a few 
lines), every one is subtly different, and there is locking going on.


PS5, Line 874: // Update the entry TTL.
 : auto& entry = FindOrDie(tablets_by_k
> What if the total number of tablets is a multiple of MAX_RETURNED_TABLE_LOC
correct, that's exactly how it should work (we can't infer the presence of a 
non-covered range in that case).  It will be discovered if a lookup is 
requested for that range.


Line 885:  
tablets_by_key.lower_bound(tablet_upper_bound));
> Why FindFloorOrNull if you DCHECK_NOTNULL() right after? Seems unsafe.
There is no FindFloorOrDie.  I've updated to be a normal CHECK.  I originally 
had it as a DCHECK since it should only fire if there are bugs in the code, but 
I doubt in makes much of a performance difference.


http://gerrit.cloudera.org:8080/#/c/3581/5/src/kudu/client/table-internal.cc
File src/kudu/client/table-internal.cc:

PS5, Line 133: if (s.ok()) {
 :   break;
 : } else {
 :   LOG(WARNING) << "Error getting table locations: " << 
s.ToString() << ", retrying.";
 : }
> Likewise, this isn't logically part of this change, right?
Right, same scenario.


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

Line 150: DEFINE_int32(table_locations_ttl_ms, 60 * 60 * 1000, // 1 hour
> How about expressing this as:
Done


http://gerrit.cloudera.org:8080/#/c/3581/5/src/kudu/tools/ksck_remote.cc
File src/kudu/tools/ksck_remote.cc:

Line 283:   SleepFor(MonoDelta::FromMilliseconds(100 * retries));
> That seems like a rather high sleep value. Does it cause the ksck tests to 
Done


Line 320:   new KsckTabletReplica(replica.ts_info().permanent_uuid(), 
is_leader, is_follower)));
> Nit: wasn't the old indentation correct?
Done


-- 
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: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert

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

Adar Dembo 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 ClearCache() 
should be set up as a ScopedCleanup thing right after L888-889.


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

Also, not sure about "sufficient"; it implies that there was something more we 
could do but have chosen not to. Maybe you meant "necessary"?


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 
same range? Is that legal, provided they're in that order?


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


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?


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 
combined. The overhead of two unique_ptrs per Step for non-partitioning steps 
is minimal, and it'll simplify ToRequest() somewhat.


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?


Line 459:   scanner.SetTimeoutMillis(6);
What's the significance of this value?


Line 510:   vector master_addrs_;
Unused?


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 
AlterTable() that adds a partition, drops another, and adds a column too.


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.


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


Line 463:   session->SetTimeoutMillis(15 * 1000);
Why this value?


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


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?


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


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

Also, what about negative test cases? Like dropping partitions that don't 
exist, adding partitions that overlap with existing ones, adding the same 
partition twice, etc.


Line 1065:   gscoped_ptr table_alterer;
unique_ptr?


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


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 
insert/scan?


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;
  :   
RETURN_NOT_OK(PartitionSchema::FromPB(table->metadata().state().pb.partition_schema(),
  : schema, 
_schema));
This isn't safe; you should be getting the schema() through the COWLocked 
object.


PS4, Line 1223: std::lock_guard l(table->lock_);
  : tablets = table->tablet_map_;
This is why you needed to

[kudu-CR] [c++-client]: cache non-covering ranges in meta cache

Dan Burkert has posted comments on this change.

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


Patch Set 5:

(1 comment)

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

Line 885:   *cache_entry = *DCHECK_NOTNULL(FindFloorOrNull(tablets_by_key, 
rpc.partition_key()));
> There is no FindFloorOrDie.  I've updated to be a normal CHECK.  I original
Woops, I did actually implement FindFloorOrDie and switch this over to it.


-- 
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: 5
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: Yes

[kudu-CR] [c++-client]: cache non-covering ranges in meta cache

Dan Burkert has posted comments on this change.

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


Patch Set 7:

(1 comment)

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

Line 1238:   // Force a lookup and ensure the entry is refreshed.
> The TTL is 25ms, so it's conceivable that on a bogged down test environment
still working on this...


-- 
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: 7
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: Yes

[kudu-CR] Move the maintenance manager to util

Adar Dembo has posted comments on this change.

Change subject: Move the maintenance manager to util
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3656/3/src/kudu/util/maintenance_manager.h
File src/kudu/util/maintenance_manager.h:

Line 243:   void GetMaintenanceManagerStatusDump(MaintenanceManagerStatusPB* 
out_pb);
Since this is a pointer, you don't even need to include 
maintenance_manager.pb.h; you could forward declare this instead.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcf072d443ac3d069bda15b9dc0f8c442b9ac5c0
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: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: Add garbage collection to ResultTracker

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

David Ribeiro Alves has posted comments on this change.

Change subject: WIP: Add garbage collection to ResultTracker
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3628/5/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:

PS5, Line 91: // If the arriving request is older than our per-client GC 
watermark, report its
:   // staleness to the client.
> Yeah that's what I meant. So is this coming back as a row error or an RPC e
coming back as an RPC error (we don't cache results for individual rows, only 
for whole batches)


-- 
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: 5
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: Yes

[kudu-CR] KUDU-1530: Update docs about OS X build dependency on Xcode package

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

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: KUDU-1530: Update docs about OS X build dependency on Xcode 
package
..

KUDU-1530: Update docs about OS X build dependency on Xcode package

Change-Id: I8962d5539f437dba8b120c70c90c1e384ed550c9
---
M docs/installation.adoc
1 file changed, 3 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8962d5539f437dba8b120c70c90c1e384ed550c9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1530: Update docs about OS X build dependency on Xcode package

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-1530: Update docs about OS X build dependency on Xcode 
package
..


Patch Set 3:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8962d5539f437dba8b120c70c90c1e384ed550c9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] KUDU-1530: Update docs about OS X build dependency on Xcode package

Adar Dembo has posted comments on this change.

Change subject: KUDU-1530: Update docs about OS X build dependency on Xcode 
package
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8962d5539f437dba8b120c70c90c1e384ed550c9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] rw mutex: prevent recursive use

Hello Dan Burkert,

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

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

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

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
---
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(-)

git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/3641/6
--
To view, visit http://gerrit.cloudera.org:8080/3641
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7ae30ec123a16c39ef0c15ee2d2176f807df03db
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Dan Burkert
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] rw mutex: prevent recursive use

Kudu Jenkins has posted comments on this change.

Change subject: rw_mutex: prevent recursive use
..


Patch Set 6:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7ae30ec123a16c39ef0c15ee2d2176f807df03db
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 
Gerrit-HasComments: No

[kudu-CR] Move the maintenance manager to util

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

David Ribeiro Alves has posted comments on this change.

Change subject: Move the maintenance manager to util
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3656/3/src/kudu/util/maintenance_manager.h
File src/kudu/util/maintenance_manager.h:

Line 243:   void GetMaintenanceManagerStatusDump(MaintenanceManagerStatusPB* 
out_pb);
> Since this is a pointer, you don't even need to include maintenance_manager
But then I'll have to fwd declare it in a bunch of places. Since this is only a 
code move mind if I drop this?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcf072d443ac3d069bda15b9dc0f8c442b9ac5c0
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: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Move the maintenance manager to util

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

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

Change subject: Move the maintenance manager to util
..


Move the maintenance manager to util

This moves the maintenance manager to util, along with its required protobufs.
This move is required as we'll need a server-level MaintenanceMananager to
run garbage collection on the ResultTracker.

Change-Id: Ifcf072d443ac3d069bda15b9dc0f8c442b9ac5c0
Reviewed-on: http://gerrit.cloudera.org:8080/3656
Reviewed-by: Jean-Daniel Cryans 
Tested-by: Kudu Jenkins
---
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/master/master.cc
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.proto
M src/kudu/tablet/tablet_mm_ops.h
M src/kudu/tablet/tablet_peer-test.cc
M src/kudu/tablet/tablet_peer_mm_ops.cc
M src/kudu/tablet/tablet_peer_mm_ops.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server.cc
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/util/CMakeLists.txt
R src/kudu/util/maintenance_manager-test.cc
R src/kudu/util/maintenance_manager.cc
R src/kudu/util/maintenance_manager.h
A src/kudu/util/maintenance_manager.proto
18 files changed, 85 insertions(+), 59 deletions(-)

Approvals:
  Jean-Daniel Cryans: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ifcf072d443ac3d069bda15b9dc0f8c442b9ac5c0
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: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] Add integration tests for replay cache with writes

Mike Percy has posted comments on this change.

Change subject: Add integration tests for replay cache with writes
..


Patch Set 32:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/3519/32/src/kudu/integration-tests/ts_itest-base.h
File src/kudu/integration-tests/ts_itest-base.h:

Line 101: // TODO remove this once we have ResultTracker GC
> There's no JIRA, the GC patch is up for review. Will remove these comments 
ok


Line 493:   void AssertNoTabletServersCrashed() {
> This is just a move from the child class. the raft itest uses this. Happy t
ok


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I35722eb1c83f97e886cfe9d6b03ed95bcd62429f
Gerrit-PatchSet: 32
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Add a way to include request ids in log-dump

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

David Ribeiro Alves has posted comments on this change.

Change subject: Add a way to include request ids in log-dump
..


Patch Set 8: Verified+1

unrelated java flakiness.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I88d7c65887a98544ee83b5b4bc0817bea7222131
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] Integrate the result tracker with writes

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

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

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 and because testing it specifically is involved, this patch
doesn't include additional tests, these will be pushed as a follow up.

Change-Id: I1fa2f8db33653960f4749237b8993baba0929893
Reviewed-on: http://gerrit.cloudera.org:8080/3449
Reviewed-by: David Ribeiro Alves 
Reviewed-by: Jean-Daniel Cryans 
Tested-by: Kudu Jenkins
---
M src/kudu/client/batcher.cc
M src/kudu/consensus/consensus.proto
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/rpc_context.cc
M src/kudu/rpc/rpc_context.h
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_service.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
M src/kudu/tserver/tserver_service.proto
25 files changed, 399 insertions(+), 41 deletions(-)

Approvals:
  David Ribeiro Alves: Looks good to me, approved
  Jean-Daniel Cryans: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I1fa2f8db33653960f4749237b8993baba0929893
Gerrit-PatchSet: 29
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: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] Add integration tests for replay cache with writes

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

Hello Kudu Jenkins,

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

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

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

Change subject: Add integration tests for replay cache with writes
..

Add integration tests for replay cache with writes

This adds a couple of new integration tests for replay cache with
writes. Both tests start multiple threads writing, independently, to
tablet servers simulaneously. The tests leverage the fact that followers
are also able to answer requests, once they are cached, and stores all
responses, which are compared at the end of the test.

Some of the requests (1/3) are "empty" writes, so that we stress the
serialization point in transaction_driver.cc without relying on row
lock serialization.

This adds two different tests, one that stresses a lot of elections
and one that crashes nodes. This is inline with other tests we already
had in raft_consensus-itest.

This also adds a new fault injection point right after the leader sends
a request. We currently have one right _before_ the leader sends
a request, but having one for after the request is sent encourages
stressing the path where a newly elected leader as both incoming
client request and ongoing replica transactions, which can possibly
race with each other if they correspond to the same write.

Finally this changes attempt_no in RequestIdPB to be an int64 instead
of just an int. While an int is more than enough in normal operation,
the new test generates many more attempts and we need a bigger number
to make sure all attempt numbers are unique.

I looped this about 1000 times, without related failures.

Change-Id: I35722eb1c83f97e886cfe9d6b03ed95bcd62429f
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/rpc/rpc_header.proto
6 files changed, 325 insertions(+), 33 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/3519/33
-- 
To view, visit http://gerrit.cloudera.org:8080/3519
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35722eb1c83f97e886cfe9d6b03ed95bcd62429f
Gerrit-PatchSet: 33
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] Memory tracking for result tracker

Kudu Jenkins has posted comments on this change.

Change subject: Memory tracking for result tracker
..


Patch Set 9:

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

-- 
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: 9
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-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Add integration tests for replay cache with writes

Kudu Jenkins has posted comments on this change.

Change subject: Add integration tests for replay cache with writes
..


Patch Set 33:

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

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

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

[kudu-CR] WIP: Add garbage collection to ResultTracker

Kudu Jenkins has posted comments on this change.

Change subject: WIP: Add garbage collection to ResultTracker
..


Patch Set 6:

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

-- 
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: 6
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] client.h: doxygen comments for C++ API

Kudu Jenkins has posted comments on this change.

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


Patch Set 6:

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

-- 
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: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] Memory tracking for result tracker

Adar Dembo has posted comments on this change.

Change subject: Memory tracking for result tracker
..


Patch Set 9:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/3627/9/src/kudu/rpc/result_tracker.cc
File src/kudu/rpc/result_tracker.cc:

Line 75: mem_tracker_->Consume(sizeof(ClientState));
If we want to make ClientState memory tracking accurate, we need to account for 
two additional things:

1. Malloc "slop". sizeof(ClientState) may not account for the actual size of 
the memory allocation used in "new ClientState()". For example, if the size of 
the struct is 40 bytes, the allocation may be the next power of 2 (i.e. 64 
bytes). We deal with this elsewhere by using kudu_malloc_usable_size(ptr) as 
the "size of object"; sometimes this is internalized inside a 
memory_footprint() method.

2. The size of any nested heap pointers. ClientState contains a map, and that 
map contains nested pointers whose memory consumption isn't being accounted 
for. Look at how Schema::name_to_index_ is managed to see how you might do that.


Line 85: mem_tracker_->Consume(sizeof(CompletionRecord));
Same issues with CompletionRecord and its nested vector of OngoingRpcs (you can 
use kudu_malloc_usable_size(vector.data()), though you  need to check that 
vector.capacity() >0 first).

I think you're already taking care of the nested protobuf elsewhere.


Line 234:   mem_tracker_->Consume(response->ByteSize());
Should use SpaceUsed(), I think.


Line 255:   mem_tracker_->Release(sizeof(OnGoingRpcInfo));
We've found that Release() in a loop is generally less efficient than adding up 
all the memory consumed in the loop and making one call to Release() at the 
end. That change reduced block manager CPU consumption pretty dramatically on 
startup.


http://gerrit.cloudera.org:8080/#/c/3627/9/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:

PS9, Line 89:   if (id != 0) {
: StrAppend(_str, " ", id);
:   }
Don't need this; each of these memtrackers is already "disambiguated" by virtue 
of having a unique parent.

That said, why bother creating a memtracker for the ResultTracker instead of 
just reusing the server memtracker? IIRC we only create separate memtrackers 
for objects that come and go (e.g. tablet, memrowset, deltamemstore).


-- 
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: 9
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] client.h: doxygen comments for C++ API

Mike Percy has posted comments on this change.

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


Patch Set 6:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/3619/6/CMakeLists.txt
File CMakeLists.txt:

Line 1000: # "make doxydocs" target
Sorry to be a buzz kill, but can we just make this target "doxygen" instead of 
"doxydocs"?


Line 1015: COMMAND make all install DESTDIR=${DOXY_CLIENT_DESTDIR}
Can this be made to work with Ninja was well? I think you just need to replace 
make with ${CMAKE_MAKE_PROGRAM}


http://gerrit.cloudera.org:8080/#/c/3619/6/docs/.gitignore
File docs/.gitignore:

Line 19
nit: spurious change


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

Line 28: #include 
This change isn't required for this patch, is it?


http://gerrit.cloudera.org:8080/#/c/3619/6/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:

Line 262: if ! `which -s doxygen` && [ ! -d $DOXYGEN_DIR ]; then
The -s command-line flag is not available on GNU systems. You can do:

if ! which doxygen >/dev/null && [ ! -d "$DOXYGEN_DIR" ]; then


-- 
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: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1530: Update docs about OS X build dependency on Xcode package

Mike Percy has posted comments on this change.

Change subject: KUDU-1530: Update docs about OS X build dependency on Xcode 
package
..


Patch Set 3: Code-Review+2

Overriding flakiness due to isolate race condition

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8962d5539f437dba8b120c70c90c1e384ed550c9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Mike Percy 
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 8:

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

-- 
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: 8
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] [c++-client]: cache non-covering ranges in meta cache

Hello Kudu Jenkins,

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

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

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

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

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

This commit introduces a few features to the meta cache, all with the aim of
making it compatible with the upcoming add/drop range partitions feature.

1) Non-covered range partitions are now cached in the meta cache.  This is
achieved by storing MetaCacheEntry objects in the meta cache's partition-key
index instead of RemoteTablets.  The MetaCacheEntry holds either a RemoteTablet,
in which case it represents a covered partition range, or it represents a
non-covered partition range.

2) Entries are now removed from the meta cache's partition-key index when it can
be determined that the entries are no longer valid from the results of a
GetTableLocations RPC.

3) A basic TTL has been added to the GetTableLocationsResponsePB so that the
client can properly refresh the meta cache when necessary. The TTL is
configurable by the master, and defaults to one hour.

Change-Id: I05bcb3fe05d51d7c455e1d68bd2baa6f3c2b9d21
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/schema.h
M src/kudu/client/table-internal.cc
M src/kudu/gutil/map-util.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/tools/ksck_remote.cc
10 files changed, 438 insertions(+), 139 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I05bcb3fe05d51d7c455e1d68bd2baa6f3c2b9d21
Gerrit-PatchSet: 8
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

[kudu-CR] Add integration tests for replay cache with writes

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

Hello Kudu Jenkins,

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

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

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

Change subject: Add integration tests for replay cache with writes
..

Add integration tests for replay cache with writes

This adds a couple of new integration tests for replay cache with
writes. Both tests start multiple threads writing, independently, to
tablet servers simulaneously. The tests leverage the fact that followers
are also able to answer requests, once they are cached, and stores all
responses, which are compared at the end of the test.

Some of the requests (1/3) are "empty" writes, so that we stress the
serialization point in transaction_driver.cc without relying on row
lock serialization.

This adds two different tests, one that stresses a lot of elections
and one that crashes nodes. This is inline with other tests we already
had in raft_consensus-itest.

This also adds a new fault injection point right after the leader sends
a request. We currently have one right _before_ the leader sends
a request, but having one for after the request is sent encourages
stressing the path where a newly elected leader as both incoming
client request and ongoing replica transactions, which can possibly
race with each other if they correspond to the same write.

Finally this changes attempt_no in RequestIdPB to be an int64 instead
of just an int. While an int is more than enough in normal operation,
the new test generates many more attempts and we need a bigger number
to make sure all attempt numbers are unique.

I looped this about 1000 times, without related failures.

Change-Id: I35722eb1c83f97e886cfe9d6b03ed95bcd62429f
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/ts_itest-base.h
M src/kudu/rpc/rpc_header.proto
6 files changed, 326 insertions(+), 33 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/3519/34
-- 
To view, visit http://gerrit.cloudera.org:8080/3519
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I35722eb1c83f97e886cfe9d6b03ed95bcd62429f
Gerrit-PatchSet: 34
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] Add integration tests for replay cache with writes

Kudu Jenkins has posted comments on this change.

Change subject: Add integration tests for replay cache with writes
..


Patch Set 34:

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

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

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

[kudu-CR] Add integration tests for replay cache with writes

Mike Percy has posted comments on this change.

Change subject: Add integration tests for replay cache with writes
..


Patch Set 34: Code-Review+2

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

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

[kudu-CR] Add a way to include request ids in log-dump

Mike Percy has posted comments on this change.

Change subject: Add a way to include request ids in log-dump
..


Patch Set 5: Code-Review+2

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

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

[kudu-CR] Add integration tests for replay cache with writes

Mike Percy has posted comments on this change.

Change subject: Add integration tests for replay cache with writes
..


Patch Set 29:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3519/29/src/kudu/rpc/rpc_header.proto
File src/kudu/rpc/rpc_header.proto:

Line 130:   required int64 attempt_no = 4;
Please squash this change into a previous commit, or squash this whole patch 
into a previous commit.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I35722eb1c83f97e886cfe9d6b03ed95bcd62429f
Gerrit-PatchSet: 29
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR](gh-pages) Replace bootstrap with CDN version

Hello Misty Stanley-Jones,

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

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

to review the following change.

Change subject: Replace bootstrap with CDN version
..

Replace bootstrap with CDN version

This has significant performance benefits.

Also, remove our hosted bootstrap files. We don't need them.

Change-Id: Iefcc5fada12e0c87fa98a3eb7faec9db205c2cf2
---
M _includes/bottom_common.html
M _includes/top_common.html
D css/bootstrap-responsive.css
D css/bootstrap-responsive.min.css
D css/bootstrap-theme.css
D css/bootstrap-theme.css.map
D css/bootstrap-theme.min.css
D css/bootstrap.css
D css/bootstrap.css.map
D css/bootstrap.min.css
D js/bootstrap.js
D js/bootstrap.min.js
12 files changed, 6 insertions(+), 10,182 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Iefcc5fada12e0c87fa98a3eb7faec9db205c2cf2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Misty Stanley-Jones

[kudu-CR](gh-pages) Replace bootstrap with CDN version

Mike Percy has posted comments on this change.

Change subject: Replace bootstrap with CDN version
..


Patch Set 1:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iefcc5fada12e0c87fa98a3eb7faec9db205c2cf2
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: gh-pages
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Misty Stanley-Jones 
Gerrit-HasComments: No

[kudu-CR] docs: Fix broken link to Introduction in navbar

Kudu Jenkins has posted comments on this change.

Change subject: docs: Fix broken link to Introduction in navbar
..


Patch Set 1:

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

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

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

[kudu-CR] docs: Fix broken link to Introduction in navbar

Hello Misty Stanley-Jones,

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

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

to review the following change.

Change subject: docs: Fix broken link to Introduction in navbar
..

docs: Fix broken link to Introduction in navbar

We moved introduction.adoc to index.adoc, but the navigation bar still
points to introduction.html

Change-Id: If4bee45e2470d9d9c09da82d4fad7fa80893c7da
---
M docs/support/jekyll-templates/document.html.erb
1 file changed, 1 insertion(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If4bee45e2470d9d9c09da82d4fad7fa80893c7da
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Misty Stanley-Jones

[kudu-CR] Make dist test.py work on a symlinked work directory

Hello Kudu Jenkins,

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

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

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

Change subject: Make dist_test.py work on a symlinked work directory
..

Make dist_test.py work on a symlinked work directory

Previously, if run inside a directory that was partially composed of
symlinks, dist_test.py would not correctly collect and modify the RPATHs
of thirdparty libraries. It would put everything in LD_LIBRARY_PATH.
This could result in errors that looked like the following when trying
to symbolize stack traces under TSAN:

/tmp/run_tha_testQiFa2H/thirdparty/installed/bin/llvm-symbolizer: symbol lookup 
error:
/tmp/run_tha_testQiFa2H/build/dist-test-system-libs/libstdc++.so.6: undefined 
symbol: __tsan_init

With these changes, symbolization works.

run_dist_test.py just has a cosmetic change to rename a variable for
clarity. No functional changes.

Change-Id: I3e5f5c2f1993ceaa3b3a1d74bae16c6efa2c97d9
---
M build-support/dist_test.py
M build-support/run_dist_test.py
2 files changed, 38 insertions(+), 13 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3e5f5c2f1993ceaa3b3a1d74bae16c6efa2c97d9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon

[kudu-CR] Make dist test.py work on a symlinked work directory

Kudu Jenkins has posted comments on this change.

Change subject: Make dist_test.py work on a symlinked work directory
..


Patch Set 2:

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

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

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

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

Kudu Jenkins has posted comments on this change.

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


Patch Set 5:

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

-- 
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: Kudu Jenkins
Gerrit-HasComments: No

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

Kudu Jenkins has posted comments on this change.

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


Patch Set 9:

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

-- 
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

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

Kudu Jenkins has posted comments on this change.

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


Patch Set 5:

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

-- 
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: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-HasComments: No

[kudu-CR] KUDU-1530: Update docs about OS X build dependency on Xcode package

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

Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1530: Update docs about OS X build dependency on Xcode 
package
..


Patch Set 2:

(3 comments)

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

> What is the "XCode package"? Is that something that's totally obvious to a 
Package itself seems distinct from the cmdline tools, because I had cmdline 
tools installed before observing build failures. Is Xcode an obvious package 
for macOS user ? If you are building/developing on mac, I would guess so.


Line 474: The Xcode package is necessary for compiling Kudu. Some of the 
instructions
> This can probably be simplified to just say that the full xcode install is 
Cool, thanks.


Line 476: manual dependency installation is possible.
> Nit: "...that the Xcode package is..."
I figured it's better to remove cli tools references, and just say Xcode needed 
as Dan is suggesting.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8962d5539f437dba8b120c70c90c1e384ed550c9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

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

Jean-Daniel Cryans 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 16: Code-Review+2

-- 
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: 16
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-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Move the maintenance manager to util

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

David Ribeiro Alves has uploaded a new change for review.

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

Change subject: Move the maintenance manager to util
..

Move the maintenance manager to util

This moves the maintenance manager to util, along with it's required protobufs.
It doesn't change the namespace (kudu::) since that would be more involved.

Change-Id: Ifcf072d443ac3d069bda15b9dc0f8c442b9ac5c0
---
M src/kudu/integration-tests/full_stack-insert-scan-test.cc
M src/kudu/master/master.cc
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.proto
M src/kudu/tablet/tablet_mm_ops.h
M src/kudu/tablet/tablet_peer-test.cc
M src/kudu/tablet/tablet_peer_mm_ops.cc
M src/kudu/tablet/tablet_peer_mm_ops.h
M src/kudu/tserver/mini_tablet_server.cc
M src/kudu/tserver/tablet_server-test-base.h
M src/kudu/tserver/tablet_server.cc
M src/kudu/tserver/tserver-path-handlers.cc
M src/kudu/util/CMakeLists.txt
R src/kudu/util/maintenance_manager-test.cc
R src/kudu/util/maintenance_manager.cc
R src/kudu/util/maintenance_manager.h
A src/kudu/util/maintenance_manager.proto
18 files changed, 83 insertions(+), 57 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifcf072d443ac3d069bda15b9dc0f8c442b9ac5c0
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves

[kudu-CR] Add integration tests for replay cache with writes

Mike Percy has posted comments on this change.

Change subject: Add integration tests for replay cache with writes
..


Patch Set 24:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3519/24/src/kudu/integration-tests/raft_consensus-itest.cc
File src/kudu/integration-tests/raft_consensus-itest.cc:

PS24, Line 1184:  defined(__SANITIZE_ADDRESS__) || defined(ADDRESS_SANITIZER)
> gcc calls it one thing and clang calls it another. We've used the two elsew
We should only need -DADDRESS_SANITIZER because it's something we put in. See 
/CMakeLists.txt L301


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I35722eb1c83f97e886cfe9d6b03ed95bcd62429f
Gerrit-PatchSet: 24
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

Dan Burkert has posted comments on this change.

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


Patch Set 5: Code-Review+2

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

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

Dan Burkert has posted comments on this change.

Change subject: rw_mutex: prevent recursive use
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7ae30ec123a16c39ef0c15ee2d2176f807df03db
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] KUDU-1374: send full tablet report when new leader master is detected

Adar Dembo has posted comments on this change.

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


Patch Set 5: Verified+1

More chrpath() and isolate failures.

-- 
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: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] master: fix corruption when AlterTable() races with CreateTable()

Adar Dembo has submitted this change and it was merged.

Change subject: master: fix corruption when AlterTable() races with 
CreateTable()
..


master: fix corruption when AlterTable() races with CreateTable()

Admittedly, this is a contrived scenario:
1. T1 tries to create table with name 'foo'
2. T2 tries to rename table with name 'bar' to 'foo'

With just the right timing, both operations succeed and the metadata now has
two tables named 'foo', each with a different table ID. The fix is simple:
generalize the "tables being created" logic already used by CreateTable().

Without the fix, the new test failed every 50th run or so. With it, it
doesn't fail in 1000 runs.

Change-Id: I6c9e4214c09bc47a5a10b12d6ffe8b35906708c9
Reviewed-on: http://gerrit.cloudera.org:8080/3607
Reviewed-by: Dan Burkert 
Tested-by: Adar Dembo 
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master-test.cc
3 files changed, 231 insertions(+), 30 deletions(-)

Approvals:
  Dan Burkert: Looks good to me, approved
  Adar Dembo: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I6c9e4214c09bc47a5a10b12d6ffe8b35906708c9
Gerrit-PatchSet: 8
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

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

Jean-Daniel Cryans has posted comments on this change.

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


Patch Set 26: Code-Review+2

-- 
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: 26
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: No

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

Jean-Daniel Cryans 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 15: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/3506/15/src/kudu/integration-tests/ts_itest-base.h
File src/kudu/integration-tests/ts_itest-base.h:

PS15, Line 102: =true
nit: you don't need that.


-- 
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: 15
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-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

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

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

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

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


Integrate the request tracker with the client

This integrates the request tracker with the client, making sure
that write requests sent by the client are tracked in the RequestTracker
and contain the information necessary for exactly once semantics.

This adds to rpc-stress-test a test that uses RetriableRpc, instead
of using the proxy directly.

Change-Id: I94207c294452fcbdb3a7901fdb702674d47553ee
Reviewed-on: http://gerrit.cloudera.org:8080/3080
Tested-by: Kudu Jenkins
Reviewed-by: Jean-Daniel Cryans 
---
M src/kudu/client/batcher.cc
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client.cc
M src/kudu/rpc/retriable_rpc.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc_controller.cc
M src/kudu/rpc/rpc_controller.h
M src/kudu/rpc/rtest.proto
10 files changed, 244 insertions(+), 6 deletions(-)

Approvals:
  Jean-Daniel Cryans: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I94207c294452fcbdb3a7901fdb702674d47553ee
Gerrit-PatchSet: 27
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-Reviewer: Todd Lipcon

[kudu-CR] KUDU-1530: Update docs about OS X build dependency on Xcode package

Kudu Jenkins has posted comments on this change.

Change subject: KUDU-1530: Update docs about OS X build dependency on Xcode 
package
..


Patch Set 2:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8962d5539f437dba8b120c70c90c1e384ed550c9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: 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 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 16:

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

-- 
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: 16
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-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

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

Dan Burkert has posted comments on this change.

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


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3610/6/src/kudu/integration-tests/ts_tablet_manager-itest.cc
File src/kudu/integration-tests/ts_tablet_manager-itest.cc:

Line 176: if (reports[0].updated_tablets_size()) break;
!reports[0].updated_tablets().empty()


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

Line 206: for (int i = first_failure_idx - 1; i >= 0; i--) {
I think this would be more straightforward as a forward loop:

for (int i = 0; i < first_failure_idx; i++)

you might also simplify some of the locals by putting it inside the failure 
claus in the first loop.


Line 384: "Failed to send heartbeat to master");
should this have the master addr as well?


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

Line 153:   void PopulateTabletReport(master::TabletReportPB* report,
this API is pretty funky, any reason not to have separate 
PopulateFullTabletReport and PopulatePartialTabletReport(TabletReportPB*, const 
vector&) methods?  Doesn't look like the implementation reuses much.


-- 
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: 6
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] Memory tracking for result tracker

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

Hello Kudu Jenkins,

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

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

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

Change subject: Memory tracking for result tracker
..

Memory tracking for result tracker

This adds memory tracking to ResultTracker, making sure we account for the 
memory
as we cache responses for client's requests.

Testing wise this adds memory consumption checks to rpc-stress-test.cc.

Change-Id: I3b81dda41c8bc7f70380ce426142c34afe6f1625
---
M src/kudu/rpc/result_tracker.cc
M src/kudu/rpc/result_tracker.h
M src/kudu/rpc/rpc-stress-test.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/server/server_base.cc
5 files changed, 106 insertions(+), 15 deletions(-)


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

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

[kudu-CR] Add a way to include request ids in log-dump

Kudu Jenkins has posted comments on this change.

Change subject: Add a way to include request ids in log-dump
..


Patch Set 6:

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

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

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

[kudu-CR] [java client] Integrate with the replay cache

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

David Ribeiro Alves has posted comments on this change.

Change subject: [java client] Integrate with the replay cache
..


Patch Set 4:

(4 comments)

looks mostly good, does this still have the side effect of making us not ignore 
duplicated rows where we did before?

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

PS4, Line 107: long
docs


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

Line 163:   boolean isRetryableRpc() {
docs


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

Line 41:   public RequestTracker(String clientId) {
does the request tracker need to have the client id?


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

PS4, Line 254: 
requestIdBuilder.setFirstIncompleteSeqNo(requestTracker.firstIncomplete());
> In the previous patch I wasn't sending this if the value would have been NO
thanks


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I108cd30acbc308bfb4577d072c2a8f26d1553c68
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Jean-Daniel Cryans 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

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

2016-07-14 Thread Alexey Serbin (Code Review)

Hello Kudu Jenkins,

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

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

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

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

doxygen for C++ client API

If doxygen is available, build 'doxygen' taget to generate
Doxygen docs from client.h and other Kudu C++ client API files,
After the target is built, open
${CMAKE_CURRENT_BINARY_DIR}/docs/doxygen/client_api/html/index.html
in your favorite browser to see the generated documentation.

If doxygen is available, the 'docs' target generates
doxygen documentaion as well since it depends on the 'doxygen' target.

Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
---
M CMakeLists.txt
A docs/support/doxygen/client_api.doxy.in
M src/kudu/client/client.h
M thirdparty/download-thirdparty.sh
4 files changed, 1,294 insertions(+), 716 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/3619/8
-- 
To view, visit http://gerrit.cloudera.org:8080/3619
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie7d42fb1c90b83074e357dcecf42489ed9fc4f02
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dinesh Bhat 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy

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