[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Hao Hao (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..

KUDU-1943: Add BlockTransaction to Block Manager

This adds a new layer of abstraction 'BlockTransaction' to
Block Manager, to accumulate new blocks in a logical operation,
e.g. flush/compaction. Avoid using fsync() per block can
potentionaly reduce a lot I/O overhead.

This patch also add a new API, Finalize(), to Block Manager,
so that for LBM container can be reused without flushing
in-flight writable blocks to disk. This can bring down the
log block container number and disk space consumption for
first few flush/compaction operations when using log block
manager.

I performed manual test which creates a table with two tablets,
'kudu perf loadgen localhost: --num-rows-per-thread=2000
 --num-threads=10 --keep-auto-table --table_num_buckets=2'.
With this patch, 'log_block_manager_containers' dropped from 56
to 7.

A design doc can be found here:
https://docs.google.com/document/d/1cB8pDs-Ho7p2HlzkWo1d-wmXr90YntIjdvdOv7u7gn0/edit?usp=sharing

Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tablet/multi_column_writer.h
20 files changed, 591 insertions(+), 310 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 18
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: KUDU-2032 (part 1): pass pre-resolution hostname into RPC 
proxies
..


Patch Set 2:

(5 comments)

some nits

http://gerrit.cloudera.org:8080/#/c/7687/2/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:

PS2, Line 219: addrs.size()
nit: while you are at it, would it make sense to update this to

if (addrs.empty()) ?


http://gerrit.cloudera.org:8080/#/c/7687/2/src/kudu/master/ts_descriptor.h
File src/kudu/master/ts_descriptor.h:

PS2, Line 129: std::string* host
nit: would it make sense to have the second parameter optional, i.e. change the 
signature to

Status ResolveSockaddr(Sockaddr* addr, std::string* host = nullptr);

?


http://gerrit.cloudera.org:8080/#/c/7687/2/src/kudu/rpc/connection_id.h
File src/kudu/rpc/connection_id.h:

PS2, Line 67:   
nit: const?  Or it's not feasible because of id0 = id1 assignments somewhere?

If adding the 'const' modifier is feasible, consider adding it for other 
members as well.


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

PS2, Line 61:   tserver::TabletServer::kDefaultPort, ));
:   generic_proxy_.reset(new server::GenericServiceProxy(
:   messenger_, addresses[0], host_port_.host()));
:   ts_proxy_.reset(new tserver::TabletServerServiceProxy(
:   messenger_, addresses[0], host_port_.host()));
:   consensus_proxy_.reset(new consensus::ConsensusServiceProxy(
:   messenger_, addresses[0], host_port_.host()));
style nit: maybe, introduce variables (references) to address[0] and 
host_port_.host() and use those as arguments for 3 proxy constructors?


http://gerrit.cloudera.org:8080/#/c/7687/2/src/kudu/util/net/sockaddr.cc
File src/kudu/util/net/sockaddr.cc:

PS2, Line 110: string(
nit: consider dropping this for brevity:

return str;


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I96fb3c73382f0be6e30e29ae2e7176be42f3bb98
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..


Patch Set 17:

(10 comments)

I think I'm done reviewing everything outside log_block_manager.cc. Still a lot 
to take in there. I've been making my way through it, but thought I'd publish 
my comments so far.

http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/cfile/cfile-test.cc
File src/kudu/cfile/cfile-test.cc:

Line 892: TEST_P(TestCFileBothCacheTypes, TestReleaseBlock) {
What happened to parameterizing this test for all block_time_to_flush values? 
In my earlier comment I suggested that doing so would be moot only if there was 
no block_time_to_flush-specific code in here. But there is such code, so we 
should do the parameterization so as to avoid bit-rot.


http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/cfile/cfile_writer.cc
File src/kudu/cfile/cfile_writer.cc:

PS17, Line 266:   if (FLAGS_block_time_to_flush == "finalize") {
  : RETURN_NOT_OK(block_->Finalize());
  :   } else if (FLAGS_block_time_to_flush != "close" &&
  :  FLAGS_block_time_to_flush != "none") {
  : LOG(FATAL) << "Unknown value for block_time_to_flush: "
  :<< FLAGS_block_time_to_flush;
  :   }
This is incorrect. We should always Finalize() the block; whether or not we 
flush inside Finalize() is determined by the block manager (and the value of 
this gflag).


http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/fs/block_manager.cc
File src/kudu/fs/block_manager.cc:

Line 43: DEFINE_string(block_time_to_flush, "finalize",
> warning: redundant 'FLAGS_block_time_to_flush' declaration [readability-red
Let's rename to "block_manager_flush_control":
1. "block_manager_" is the prefix we've been using for other BM-related flags.
2. "time_to_flush" sounds conspicuously like time to live and other such 
numerical values.


Line 44:   "When to flush blocks registered in a BlockTransaction. "
This should apply to any WritableBlock, not just those in a BlockTransaction.


PS17, Line 48: none
Let's use "never" instead, since we're talking about a point in time.


http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:

Line 32: DECLARE_string(block_time_to_flush);
This doesn't belong here (block_coalesce_close didn't belong either).


http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/fs/file_block_manager.cc
File src/kudu/fs/file_block_manager.cc:

Line 340: 
RETURN_NOT_OK_HANDLE_ERROR(writer_->Flush(WritableFile::FLUSH_ASYNC));
This is where we should check the value of block_time_to_flush. We should only 
flush if it's 'finalize'.


http://gerrit.cloudera.org:8080/#/c/7207/17/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 1176: LOG(WARNING) << Substitute(
I mentioned this to Dan in our impromptu meeting yesterday; perhaps you weren't 
yet there: we should remove this WARNING because with out-of-order block 
metadata on disk it'll fire more frequently.


PS17, Line 1397: container_->block_manager()
You can use 'lbm' here.


Line 1410: RETURN_NOT_OK(container_->FlushData(block_offset_, 
block_length_));
And here we also need to gate on block_time_to_flush == 'finalize'.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] [docs] Deprecate Java 7 and Spark 1

[Grant Henke (Code Review)]

Grant Henke has uploaded a new change for review.

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

Change subject: [docs] Deprecate Java 7 and Spark 1
..

[docs] Deprecate Java 7 and Spark 1

Starts the release notes for Kudu 1.5.0 and adds
entries for Java 7 and Spark 1 deprecation.

Change-Id: I050df790ca51729cc99866b9cd75933c3c907a4c
---
M docs/prior_release_notes.adoc
M docs/release_notes.adoc
2 files changed, 254 insertions(+), 189 deletions(-)


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

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

[kudu-CR] [iwyu] update on the internal and boost mappings

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [iwyu] update on the internal and boost mappings
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7696/1/thirdparty/patches/llvm-iwyu-include-picker.patch
File thirdparty/patches/llvm-iwyu-include-picker.patch:

> even if the number of patches is the same?
Oh, I see.  That's the only way to refresh stale workspaces.  All right, I'll 
bump the patch level up.


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

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

[kudu-CR] [iwyu] update on the internal and boost mappings

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: [iwyu] update on the internal and boost mappings
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7696/1/build-support/iwyu/mappings/boost-all.imp
File build-support/iwyu/mappings/boost-all.imp:

Line 960: { include: ["", private, 
"", public ] },
> Are these also imported from upstream? Or your changes?
These are my changes.  OK, keeping them separate would be a better choice, 
agreed.  From the other side, I could try to re-generate the whole file.


http://gerrit.cloudera.org:8080/#/c/7696/1/thirdparty/patches/llvm-iwyu-include-picker.patch
File thirdparty/patches/llvm-iwyu-include-picker.patch:

> You need to bump LLVM's patchlevel for this to be incorporated in existing 
even if the number of patches is the same?


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

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

[kudu-CR] [iwyu] update on the internal and boost mappings

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [iwyu] update on the internal and boost mappings
..


Patch Set 1:

(3 comments)

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

PS1, Line 11: IWUY
IWYU


http://gerrit.cloudera.org:8080/#/c/7696/1/build-support/iwyu/mappings/boost-all.imp
File build-support/iwyu/mappings/boost-all.imp:

Line 960: { include: ["", private, 
"", public ] },
Are these also imported from upstream? Or your changes?

If the latter, perhaps we should put them in a separate file so it's easier to 
apply updates from upstream.


http://gerrit.cloudera.org:8080/#/c/7696/1/thirdparty/patches/llvm-iwyu-include-picker.patch
File thirdparty/patches/llvm-iwyu-include-picker.patch:

You need to bump LLVM's patchlevel for this to be incorporated in existing 
deployments.


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

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

[kudu-CR] [iwyu] update on the internal and boost mappings

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new change for review.

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

Change subject: [iwyu] update on the internal and boost mappings
..

[iwyu] update on the internal and boost mappings

Updated the internal and boost mappings to avoid some bogus suggestions
generated by the include-what-you-use tool.  Also, shortened the list
of files where IWUY suggestions are 'muted'.

Change-Id: I9a20616093f362a5b5ae30627b6121313b50efa2
---
M build-support/iwyu/iwyu-filter.awk
M build-support/iwyu/mappings/boost-all.imp
M thirdparty/patches/llvm-iwyu-include-picker.patch
3 files changed, 21 insertions(+), 103 deletions(-)


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

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

[kudu-CR] [iwyu] first pass

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: [iwyu] first pass
..

[iwyu] first pass

Updated C++ source files in accordance with some of the recommendations
from include-what-you-use (IWYU) tool:
  * remove unused header files
  * add missing header files
  * use forward declarations when possible

As a result, time of compilation reduced ~10% if building with GNU make
in parallel with 48 jobs (make -j48) at ve0518.halxg.cloudera.com:

prior:
  real1m46.782s
  user47m1.633s
  sys 3m10.678s

after:
  real1m36.867s
  user42m17.340s
  sys 2m54.117s

The next step is automating the checks, so IWYU check would run
automatically (like the LINT build).

NOTE:
  As of now, not all recommendations from the tool are applied yet,
  especially for the test-related code.  That's because some
  recommendations produced by IWYU are incorrect.  Basically, every
  suggestion required manual inspection and judgment, at least it
  was necessary to verify the result code at least compiles.  As a
  result, the process of applying those recommendations is tedious
  and time consuming.  Hopefully, IWYU will get better in the future.
  Meanwhile, we can address the rest of the recommendations file-by-file
  or so in the short run.

Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
---
M src/kudu/benchmarks/rle.cc
M src/kudu/benchmarks/tpch/line_item_tsv_importer.h
M src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc
M src/kudu/benchmarks/tpch/rpc_line_item_dao.cc
M src/kudu/benchmarks/tpch/rpc_line_item_dao.h
M src/kudu/benchmarks/tpch/tpch1.cc
M src/kudu/benchmarks/tpch/tpch_real_world.cc
M src/kudu/benchmarks/wal_hiccup.cc
M src/kudu/cfile/binary_dict_block.cc
M src/kudu/cfile/binary_dict_block.h
M src/kudu/cfile/binary_plain_block.cc
M src/kudu/cfile/binary_plain_block.h
M src/kudu/cfile/binary_prefix_block.cc
M src/kudu/cfile/binary_prefix_block.h
M src/kudu/cfile/block_cache-test.cc
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/block_cache.h
M src/kudu/cfile/block_compression.cc
M src/kudu/cfile/block_compression.h
M src/kudu/cfile/bloomfile-test-base.h
M src/kudu/cfile/bloomfile-test.cc
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/bshuf_block.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/cfile_reader.h
M src/kudu/cfile/cfile_util.cc
M src/kudu/cfile/cfile_util.h
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/index-test.cc
M src/kudu/cfile/index_block.cc
M src/kudu/cfile/index_block.h
M src/kudu/cfile/index_btree.cc
M src/kudu/cfile/index_btree.h
M src/kudu/cfile/mt-bloomfile-test.cc
M src/kudu/cfile/type_encodings.cc
M src/kudu/cfile/type_encodings.h
M src/kudu/client/batcher.cc
M src/kudu/client/batcher.h
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/client-test-util.cc
M src/kudu/client/client-test-util.h
M src/kudu/client/client-test.cc
M src/kudu/client/client-unittest.cc
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client_builder-internal.h
M src/kudu/client/error-internal.cc
M src/kudu/client/error-internal.h
M src/kudu/client/error_collector.cc
M src/kudu/client/error_collector.h
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/partitioner-internal.cc
M src/kudu/client/partitioner-internal.h
M src/kudu/client/predicate-test.cc
M src/kudu/client/replica-internal.cc
M src/kudu/client/replica-internal.h
M src/kudu/client/resource_metrics.cc
M src/kudu/client/resource_metrics.h
M src/kudu/client/samples/sample.cc
M src/kudu/client/scan_batch.cc
M src/kudu/client/scan_batch.h
M src/kudu/client/scan_configuration.cc
M src/kudu/client/scan_configuration.h
M src/kudu/client/scan_predicate.cc
M src/kudu/client/scan_predicate.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
M src/kudu/client/scanner-internal.cc
M src/kudu/client/scanner-internal.h
M src/kudu/client/schema-internal.h
M src/kudu/client/schema.cc
M src/kudu/client/schema.h
M src/kudu/client/session-internal.cc
M src/kudu/client/session-internal.h
M src/kudu/client/shared_ptr.h
M src/kudu/client/table-internal.cc
M src/kudu/client/table-internal.h
M src/kudu/client/table_alterer-internal.cc
M src/kudu/client/table_alterer-internal.h
M src/kudu/client/table_creator-internal.cc
M src/kudu/client/table_creator-internal.h
M src/kudu/client/tablet-internal.h
M src/kudu/client/tablet_server-internal.cc
M src/kudu/client/tablet_server-internal.h
M src/kudu/client/value-internal.h
M src/kudu/client/value.cc
M src/kudu/client/value.h
M src/kudu/client/write_op.cc
M 

[kudu-CR] KUDU-1942. Kerberos fails to log in on hostnames with capital letters

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: KUDU-1942. Kerberos fails to log in on hostnames with capital 
letters
..


Patch Set 2:

(2 comments)

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

PS2, Line 12: With this change I was able to set up a working cluster on my 
laptop
: with a capitalized hostname, where before it would fail as 
described in
: the JIRA.
Is it worth adding a small test to catch a regression, if any?


http://gerrit.cloudera.org:8080/#/c/7693/2/src/kudu/security/init.cc
File src/kudu/security/init.cc:

PS2, Line 20: #include 
nit: maybe, #include  ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5ef65dd827459476a2d225d8e3f7c80ff2fdf627
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2032 (part 2): propagate master hostnames into client

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2032 (part 2): propagate master hostnames into client
..

KUDU-2032 (part 2): propagate master hostnames into client

This changes the client code to remember the user-specified master
addresses and propagate them into the creation of master proxies.

It's not possible to reproduce the necessary DNS configurations in a
minicluster test, but with this patch I am now able to use 'kudu perf
loadgen' against a Kerberized cluster even when my local krb5.conf has
rdns=false.

Change-Id: Ie5838f22c96f757124112b505825a53740468ce1
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/integration-tests/external_mini_cluster.cc
5 files changed, 56 insertions(+), 39 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie5838f22c96f757124112b505825a53740468ce1
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] rpc: move ConnectionId to its own file

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: rpc: move ConnectionId to its own file
..


rpc: move ConnectionId to its own file

This class was previously implemented inside of outbound_call.{cc,h}
where it didn't quite belong. In the several years since the code was
initially written we've moved more towards a "single class per header"
model unless the classes are truly trivial or really tightly coupled.
Neither is the case here.

Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9
Reviewed-on: http://gerrit.cloudera.org:8080/7685
Reviewed-by: Alexey Serbin 
Tested-by: Todd Lipcon 
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/connection_id.cc
A src/kudu/rpc/connection_id.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/proxy.h
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc_context.cc
8 files changed, 174 insertions(+), 120 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] rpc: move ConnectionId to its own file

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: rpc: move ConnectionId to its own file
..


Patch Set 2: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] gutil: remove use of deprecated headers

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: gutil: remove use of deprecated headers
..


gutil: remove use of deprecated headers

We've been on C++11 for a long time now. This commit removes usage of
ext/hash_map and ext/hash_set headers in favor of the now-standardized
unordered_map and unordered_set.

Additionally, it removes our specializations of __gnucxx::hash<> and
instead changes to specializing std::hash<> where appropriate. Some of
those specializations are now part of the standard and have been removed
as necessary to fix compilation.

This does not yet remove the -Wno-deprecated setting in the top-level
CMakeLists.txt, since it appears we have a lot of usage of our own
internal deprecated functions.

Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b
Reviewed-on: http://gerrit.cloudera.org:8080/7684
Tested-by: Todd Lipcon 
Reviewed-by: Alexey Serbin 
---
M CMakeLists.txt
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/hash/hash.cc
M src/kudu/gutil/hash/hash.h
M src/kudu/gutil/strings/serialize.cc
M src/kudu/gutil/strings/serialize.h
M src/kudu/gutil/strings/split.cc
M src/kudu/gutil/strings/split.h
9 files changed, 40 insertions(+), 126 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] security: only lookup hostname if HOST substitution is required

[Todd Lipcon (Code Review)]

Hello Sailesh Mukil,

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

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

to review the following change.

Change subject: security: only lookup hostname if _HOST substitution is required
..

security: only lookup hostname if _HOST substitution is required

The Kerberos principal configuration uses the special token '_HOST' to
indicate that the FQDN of the host should be specified. Previously we
would always lookup the FQDN even if the substitution was not required,
which might mean that startup would fail if there was no FQDN available,
even if no _HOST substitution was required.

Now, we only lookup the FQDN if FLAGS_principal contains the
substitution token. This provides the possibility of a workaround of
explicit principal configuration on machines with no FQDN.

Change-Id: I5de8647d6cf63ea70d880fa530fa289e8bae24fe
---
M src/kudu/security/init.cc
1 file changed, 10 insertions(+), 7 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5de8647d6cf63ea70d880fa530fa289e8bae24fe
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Sailesh Mukil 

[kudu-CR] KUDU-1942. Kerberos fails to log in on hostnames with capital letters

[Todd Lipcon (Code Review)]

Hello Sailesh Mukil,

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

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

to review the following change.

Change subject: KUDU-1942. Kerberos fails to log in on hostnames with capital 
letters
..

KUDU-1942. Kerberos fails to log in on hostnames with capital letters

This ensures that servers canonicalize their FQDNs to lower-case before
generating Kerberos principal names.

With this change I was able to set up a working cluster on my laptop
with a capitalized hostname, where before it would fail as described in
the JIRA.

I also verified that I was able to connect from both C++ and Java
clients.

Change-Id: I5ef65dd827459476a2d225d8e3f7c80ff2fdf627
---
M src/kudu/security/init.cc
1 file changed, 3 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5ef65dd827459476a2d225d8e3f7c80ff2fdf627
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Sailesh Mukil 

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..


Patch Set 4:

(4 comments)

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

Line 254:   // sidecars attached to the call as it may result in 
use-after-free of the sidecars.
> per comment in header, I think it's worth explaining why this is reasonable
Added some explanation in the header. Refer to it here.


PS3, Line 273: outbound transfer for this call so references to the sidecars
 :   // can be relinquished.
> I think this is no longer necessary to be added since the explanation of th
Done


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

PS3, Line 304: e.g. older version of Kudu server), it's fatal for the
> it's not clear what behavior this is implying. If it will FATAL, I think we
Done


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

Line 223: return sidecar_byte_size_ > 0;
> this isn't quite accurate to what the description says. It is possible to h
Right. Comments updated. Renamed function to HasNonEmptySidecars().


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

[Michael Ho (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..

KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

Previously, when an outbound call times out after transmission
of the request has begun but not finished, the RPC layer will
continue to send the entire payload (including any sidecars) to
the remote destination until the promised number of bytes specified
in the header are sent. This is problematic for the users of the
RPC layer as there is no well defined point in which the sidecars
are no longer referenced by the RPC layer. The only model in which
this would work is for the caller to either transfer or share ownership
of the sidecars with the RPC layer. This has caused some life cycle
issues with row batches in Impala before (e.g. IMPALA-5093).

This change fixes the problem above by modifying the RPC protocol
to allow a mid-transmission RPC call to be cancelled. Specifically,
a new footer is added to all outbound call requests. It contains
a flag, when true, indicates the outbound call was cancelled after
it has started but before it finished. The server will inspect this
flag and ignore inbound calls with this flag set. This footer enables
the caller to relinquish references to the sidecars early when an
outbound call is cancelled or timed-out. Once the call is cancelled
or timed-out, the RPC layer will send the remainder of the bytes for
the request with some dummy bytes. Since the server always ignores
cancelled call requests, it's okay to send random bytes.

To avoid breaking compatibility, a new RPC feature flag REQUEST_FOOTERS
is introduced in this change. During connection negotiation, the
client will always include this flag in its feature set. A server which
supports parsing footer will include REQUEST_FOOTERS in its feature set
if it sees the client also supports it. A client will only send the
footer if the remote server has the RPC feature flag REQUEST_FOOTERS.

Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
---
M docs/design-docs/rpc.md
M src/kudu/rpc/blocking_ops.cc
M src/kudu/rpc/connection.cc
M src/kudu/rpc/connection.h
M src/kudu/rpc/constants.cc
M src/kudu/rpc/inbound_call.cc
M src/kudu/rpc/inbound_call.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-test-base.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_header.proto
M src/kudu/rpc/rtest.proto
M src/kudu/rpc/serialization.cc
M src/kudu/rpc/serialization.h
M src/kudu/rpc/server_negotiation.cc
M src/kudu/rpc/transfer.cc
M src/kudu/rpc/transfer.h
19 files changed, 532 insertions(+), 160 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] gutil: remove use of deprecated headers

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: gutil: remove use of deprecated headers
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] separate DataDirManager from BlockManagers

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: separate DataDirManager from BlockManagers
..


Patch Set 10: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: separate DataDirManager from BlockManagers
..

separate DataDirManager from BlockManagers

Currently, the DataDirManager is owned by the BlockManagers. Since only
blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under
the roots specified by 'fs_data_dirs'), this hierarchy made sense.
However, it would be nice to track other files that fall within
'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the
directory manager from the block manager makes this more feasible.

This patch introduces a number of changes to the FsManager,
BlockManager, and DataDirManager with respect to initialization.
- BlockManagers no longer have a Create() function
- BlockManager's dtor will now just wait for DataDir closures to finish
  instead of shutting down the DataDirManager as to not directly affect
  the DataDirManager
- DataDirManagers now have two static constructors: one to open an
  existing layout, and another to create and open a new layout
- FsManagers will only create the BlockManager when opening an fs layout
- FsManagers will only Open() a DataDirManager if one has not already
  been constructed
- A validator is added to check the value of FLAGS_block_manager
  before any of the above initialization can occur

Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
16 files changed, 443 insertions(+), 306 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] gutil: remove use of deprecated headers

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: gutil: remove use of deprecated headers
..


Patch Set 2: Verified+1

some java flake

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] rpc: some small cleanup in ConnectionId

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: rpc: some small cleanup in ConnectionId
..


Patch Set 2: Code-Review+2

It seems there were unrelated flaky tests in RELEASE and SAN configs, but 
overall LGTM.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0788052f8c943ef102f3f551a85a8b219c65c361
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] separate DataDirManager from BlockManagers

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: separate DataDirManager from BlockManagers
..


Patch Set 9: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7602/9/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

Line 381:   metadata_.swap(metadata);
Why is this (and the other changes to 'metadata' in this function) necessary? 
Why can't we just populate metadata_ in Open() as before?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] rpc: move ConnectionId to its own file

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: rpc: move ConnectionId to its own file
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: separate DataDirManager from BlockManagers
..


Patch Set 8:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/block_manager-stress-test.cc
File src/kudu/fs/block_manager-stress-test.cc:

PS8, Line 144: 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
> Don't we need to preserve this in some capacity?
Done


http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:

PS8, Line 231: qcquired
> acquired
Done


http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

Line 164
> Maybe restore this comment?
Done


Line 71:   return value == "log" || value == "file";
> On macOS can you set this up so that "file" is the only valid option?
Done


Line 270: RETURN_NOT_OK(DataDirManager::OpenExisting(env_,
> Hmm, but in the case of FsManager::CreateNewFileSystemLayout followed by Fs
Yeah, it actually seems that this is mandatory. I was banging my head trying to 
figure out the test failures, and it seems like the double-initting was causing 
an early release of file locks.

Anyway, the simple fix is to not run this block if dd_manager_ has already been 
constructed. A slightly more complicated fix might require changing the 
create/open of FsManager in a similar way, but I think that would belong in a 
separate patch.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: separate DataDirManager from BlockManagers
..

separate DataDirManager from BlockManagers

Currently, the DataDirManager is owned by the BlockManagers. Since only
blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under
the roots specified by 'fs_data_dirs'), this hierarchy made sense.
However, it would be nice to track other files that fall within
'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the
directory manager from the block manager makes this more feasible.

This patch introduces a number of changes to the FsManager,
BlockManager, and DataDirManager with respect to initialization.
- BlockManagers no longer have a Create() function
- BlockManager's dtor will now just wait for DataDir closures to finish
  instead of shutting down the DataDirManager as to not directly affect
  the DataDirManager
- DataDirManagers now have two static constructors: one to open an
  existing layout, and another to create and open a new layout
- FsManagers will only create the BlockManager when opening an fs layout
- FsManagers will only Open() a DataDirManager if one has not already
  been constructed
- A validator is added to check the value of FLAGS_block_manager
  before any of the above initialization can occur

Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
17 files changed, 449 insertions(+), 311 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] KUDU-2032 (part 2): propagate master hostnames into client

[Todd Lipcon (Code Review)]

Hello Dan Burkert, Alexey Serbin,

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

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

to review the following change.

Change subject: KUDU-2032 (part 2): propagate master hostnames into client
..

KUDU-2032 (part 2): propagate master hostnames into client

This changes the client code to remember the user-specified master
addresses and propagate them into the creation of master proxies.

It's not possible to reproduce the necessary DNS configurations in a
minicluster test, but with this patch I am now able to use 'kudu perf
loadgen' against a Kerberized cluster even when my local krb5.conf has
rdns=false.

Change-Id: Ie5838f22c96f757124112b505825a53740468ce1
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
M src/kudu/client/master_rpc.cc
M src/kudu/client/master_rpc.h
M src/kudu/integration-tests/external_mini_cluster.cc
5 files changed, 50 insertions(+), 36 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie5838f22c96f757124112b505825a53740468ce1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 

[kudu-CR] KUDU-2065, KUDU-2011: Release sidecars on cancellation or timeout

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: KUDU-2065, KUDU-2011: Release sidecars on cancellation or 
timeout
..


Patch Set 3:

(4 comments)

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

Line 254:   // attached to the call or it may result in use-after-free of 
the sidecars.
per comment in header, I think it's worth explaining why this is reasonable


PS3, Line 273:  We assume that the remote supports footer or the call
 :   // doesn't have any sidecars.
I think this is no longer necessary to be added since the explanation of this 
assumption is in the CancelOutboundTransfer function itself.


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

PS3, Line 304: the assumption is that the call doesn't have any sidecar.
it's not clear what behavior this is implying. If it will FATAL, I think we 
should be explicit here (and perhaps reproduce the reasoning from our gerrit 
discussion that we didn't start using outbound sidecars until the same version 
when we introduced footers, therefore it's not possible to have a call with 
sidecars but no footer support)


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

Line 223: return sidecar_byte_size_ > 0;
this isn't quite accurate to what the description says. It is possible to have 
an empty sidecar (I think we actually do this in the case of an empty scan 
result, for example)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5c8e294090279649082eebc4f6cfb6fe858bbbfc
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..


Patch Set 16:

> (5 comments)
 > 
 > Hao, Dan, and I had a long discussion about this patch, and I
 > wanted to reproduce our conclusions here for everyone else:
 > 
 > 1. The cfile and block performance-related knobs are useful for
 > testing, but are not as important as having a good API and a robust
 > implementation. IIUC, the resplitting of FlushDataAsync and
 > Finalize was due to retaining all the existing flags; let's not do
 > this. Let's stick to the earlier approach (where Finalize implies a
 > flush), and adjust the flags if need be.
 > 
 > 2. What flags are worth preserving? We identified the need for just
 > one, which dictates when a block's data should be flushed. It has
 > three values: "finalize" (default value), "close" (defers the flush
 > to when the entire transaction is committed), and "none" (doesn't
 > flush at all, "flushing" will happen when the transaction commits
 > and files are fsynced).
 > 
 > 3. When should AppendMetadata be called? One option is to do it at
 > Finalize time. Another is to defer it to Close. The problem with
 > doing it during Finalize is that it exacerbates the "1. AppendData,
 > 2. AppendMetadata, 3. SyncData, 4. SyncMetadata" race: if we crash
 > between #2 and #3, we may end up with just the metadata on disk,
 > and if we AppendMetadata during Finalize, we have many more blocks'
 > worth of metadata that can land on disk without corresponding data.
 > 
 > 4. But AppendMetadata is called during Close, we may end up with
 > out-of-order metadata entries on disk, since transactions could
 > commit out of order. Do we care? We don't think so; the LBM
 > implementation should already be robust to this. An alternative is
 > to use an in-memory buffer to retain metadata order, but this
 > didn't seem worth the complexity.
 > 
 > 5. What to do about zero length blocks? Should their metadata be
 > written, or skipped? The answer is: it must be written, or attempts
 > to Close such blocks should fail. If for whatever reason we skipped
 > the metadata append, we could end up with block IDs whose blocks
 > can't be found on startup.

Thanks a lot Adar for the summary!

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Hao Hao (Code Review)]

Hao Hao has posted comments on this change.

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..


Patch Set 16:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/7207/16/src/kudu/cfile/cfile-test.cc
File src/kudu/cfile/cfile-test.cc:

Line 423: class TestCFileBothCacheTypesCloseTypes : public TestCFile,
> You should be able to use this to replace TestCFileBothCacheTypes. Here's w
Done


Line 449: if (desc.close_type == CLOSE) FLAGS_cfile_close_blocks_on_finish 
= true;
> If the default value of this gflag changes to true, we'll never test the fa
Done


http://gerrit.cloudera.org:8080/#/c/7207/16/src/kudu/cfile/cfile_writer.cc
File src/kudu/cfile/cfile_writer.cc:

PS16, Line 59: // - users could always change this to "close", which slows down 
throughput
 : //   but may improve write latency.
> This part isn't quite right anymore. Should the entire comment be rewritten
Done


PS16, Line 63:  
> Nit: got an extra space here.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 16
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager

[Hao Hao (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1943: Add BlockTransaction to Block Manager
..

KUDU-1943: Add BlockTransaction to Block Manager

This adds a new layer of abstraction 'BlockTransaction' to
Block Manager, to accumulate new blocks in a logical operation,
e.g. flush/compaction. Avoid using fsync() per block can
potentionaly reduce a lot I/O overhead.

This patch also add a new API, Finalize(), to Block Manager,
so that for LBM container can be reused without flushing
in-flight writable blocks to disk.

A design doc can be found here:
https://docs.google.com/document/d/1cB8pDs-Ho7p2HlzkWo1d-wmXr90YntIjdvdOv7u7gn0/edit?usp=sharing

Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
---
M src/kudu/cfile/bloomfile.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_writer.cc
M src/kudu/cfile/cfile_writer.h
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/tablet/deltafile.cc
M src/kudu/tablet/deltafile.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/multi_column_writer.cc
M src/kudu/tablet/multi_column_writer.h
20 files changed, 591 insertions(+), 309 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Hao Hao 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Hao Hao 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] rpc: move ConnectionId to its own file

[Todd Lipcon (Code Review)]

Hello Sailesh Mukil, Alexey Serbin, Kudu Jenkins,

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

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

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

Change subject: rpc: move ConnectionId to its own file
..

rpc: move ConnectionId to its own file

This class was previously implemented inside of outbound_call.{cc,h}
where it didn't quite belong. In the several years since the code was
initially written we've moved more towards a "single class per header"
model unless the classes are truly trivial or really tightly coupled.
Neither is the case here.

Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9
---
M src/kudu/rpc/CMakeLists.txt
A src/kudu/rpc/connection_id.cc
A src/kudu/rpc/connection_id.h
M src/kudu/rpc/outbound_call.cc
M src/kudu/rpc/outbound_call.h
M src/kudu/rpc/proxy.h
M src/kudu/rpc/reactor.h
M src/kudu/rpc/rpc_context.cc
8 files changed, 174 insertions(+), 120 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5343c2f6ad305b5927d71457d5d19e3799052ec9
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-2032 (part 1): pass pre-resolution hostname into RPC 
proxies
..

KUDU-2032 (part 1): pass pre-resolution hostname into RPC proxies

This modifies the constructor of RPC proxies (generated and otherwise)
to take the remote hostname in addition to the existing resolved
Sockaddr parameter. The hostname is then passed into the ConnectionId
object, and plumbed through to the SASL client in place of the IP
address that was used previously.

The patch changes all of the construction sites of Proxy to fit the new
interface. In most of the test cases, we don't have real hostnames, so
we just use the dotted-decimal string form of the remote Sockaddr, which
matches the existing behavior.

In the real call sites, we have actual host names typically specified by
the user, and in those cases we'll need to pass those into the proxy. In
a few cases, they were conveniently available in the same function that
creates the proxy. In others, they are relatively far away, so this
patch just uses the dotted-decimal string and leaves TODOs.

In the case that Kerberos is not configured, this change should have no
effect since the hostname is ignored by SASL "plain". In the case that
Kerberos is configured with 'rdns=true', they also have no effect,
because the krb5 library will resolve and reverse the hostname from the
IP as it did before. In the case that 'rdns=false', this moves us one
step closer to fixing KUDU-2032 by getting a hostname into the SASL
library.

I verified that, if I set 'rdns = false' on a Kerberized client, I'm now
able to run  'kudu master status ' successfully where it would not
before. This tool uses a direct proxy instantiation where the hostname
was easy to plumb in. 'kudu table list ' still does not work because
it uses the client, which wasn't convenient to plumb quite yet.

Given that this makes incremental improvement towards fixing the issue
without any regression, and is already a fairly wide patch, I hope to
commit this and then address the remaining plumbing in a separate patch.

Change-Id: I96fb3c73382f0be6e30e29ae2e7176be42f3bb98
---
M src/kudu/client/client-internal.cc
M src/kudu/client/master_rpc.cc
M src/kudu/client/meta_cache.cc
M src/kudu/consensus/consensus_peers.cc
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/integration-tests/exactly_once_writes-itest.cc
M src/kudu/integration-tests/external_mini_cluster.cc
M src/kudu/integration-tests/internal_mini_cluster.cc
M src/kudu/integration-tests/master-stress-test.cc
M src/kudu/integration-tests/master_cert_authority-itest.cc
M src/kudu/integration-tests/master_replication-itest.cc
M src/kudu/integration-tests/security-itest.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/ts_tablet_manager-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.cc
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M src/kudu/rpc/connection.h
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/exactly_once_rpc-test.cc
M src/kudu/rpc/mt-rpc-test.cc
M src/kudu/rpc/negotiation.cc
M src/kudu/rpc/protoc-gen-krpc.cc
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/proxy.h
M src/kudu/rpc/reactor.cc
M src/kudu/rpc/rpc-bench.cc
M src/kudu/rpc/rpc-test.cc
M src/kudu/rpc/rpc_stub-test.cc
M src/kudu/tools/ksck_remote.cc
M src/kudu/tools/tool_action_common.cc
M src/kudu/tserver/heartbeater.cc
M src/kudu/tserver/tablet_copy_client.cc
M src/kudu/tserver/tablet_copy_service-test.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/tserver/tablet_server_test_util.cc
M src/kudu/util/net/net_util-test.cc
M src/kudu/util/net/sockaddr.cc
M src/kudu/util/net/sockaddr.h
42 files changed, 232 insertions(+), 132 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I96fb3c73382f0be6e30e29ae2e7176be42f3bb98
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] rpc: some small cleanup in ConnectionId

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: rpc: some small cleanup in ConnectionId
..

rpc: some small cleanup in ConnectionId

Change-Id: I0788052f8c943ef102f3f551a85a8b219c65c361
---
M src/kudu/rpc/connection_id.cc
M src/kudu/rpc/connection_id.h
M src/kudu/rpc/proxy.cc
M src/kudu/rpc/user_credentials.cc
M src/kudu/rpc/user_credentials.h
5 files changed, 13 insertions(+), 36 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0788052f8c943ef102f3f551a85a8b219c65c361
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] gutil: remove use of deprecated headers

[Todd Lipcon (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: gutil: remove use of deprecated headers
..

gutil: remove use of deprecated headers

We've been on C++11 for a long time now. This commit removes usage of
ext/hash_map and ext/hash_set headers in favor of the now-standardized
unordered_map and unordered_set.

Additionally, it removes our specializations of __gnucxx::hash<> and
instead changes to specializing std::hash<> where appropriate. Some of
those specializations are now part of the standard and have been removed
as necessary to fix compilation.

This does not yet remove the -Wno-deprecated setting in the top-level
CMakeLists.txt, since it appears we have a lot of usage of our own
internal deprecated functions.

Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b
---
M CMakeLists.txt
M src/kudu/gutil/CMakeLists.txt
M src/kudu/gutil/endian.h
M src/kudu/gutil/hash/hash.cc
M src/kudu/gutil/hash/hash.h
M src/kudu/gutil/strings/serialize.cc
M src/kudu/gutil/strings/serialize.h
M src/kudu/gutil/strings/split.cc
M src/kudu/gutil/strings/split.h
9 files changed, 40 insertions(+), 126 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] rpc: some small cleanup in ConnectionId

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: rpc: some small cleanup in ConnectionId
..


Patch Set 1:

(1 comment)

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

PS1, Line 68: set_real_user
> nit: maybe, it's worth updating the signature of UserCredentials::set_real_
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0788052f8c943ef102f3f551a85a8b219c65c361
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] gutil: remove use of deprecated headers

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: gutil: remove use of deprecated headers
..


Patch Set 1:

(1 comment)

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

PS1, Line 214: #if defined(__GNUC__)
 : // Use our nice hash function for strings
 : template
 : struct hash > {
 :   size_t operator()(const std::basic_string<_CharT, _Traits, 
_Alloc>& k) const {
 : return HashTo32(k.data(), static_cast(k.length()));
 :   }
 : };
 : 
 : // they don't define a hash for const string at all
 : template<> struct hash {
 :   size_t operator()(const std::string& k) const {
 : return HashTo32(k.data(), static_cast(k.length()));
 :   }
 : };
 : #endif  // defined(__GNUC__)
> I guess this one causes problems when building with libc++ (worked ok with 
SGTM :)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] gutil: remove use of deprecated headers

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: gutil: remove use of deprecated headers
..


Patch Set 1:

(1 comment)

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

PS1, Line 214: #if defined(__GNUC__)
 : // Use our nice hash function for strings
 : template
 : struct hash > {
 :   size_t operator()(const std::basic_string<_CharT, _Traits, 
_Alloc>& k) const {
 : return HashTo32(k.data(), static_cast(k.length()));
 :   }
 : };
 : 
 : // they don't define a hash for const string at all
 : template<> struct hash {
 :   size_t operator()(const std::string& k) const {
 : return HashTo32(k.data(), static_cast(k.length()));
 :   }
 : };
 : #endif  // defined(__GNUC__)
> Does it make sense to remove this as well?
I guess this one causes problems when building with libc++ (worked ok with 
libstdcxx). Will remove and cross fingers :)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2098. Drop Spark 1 Support

[Grant Henke (Code Review)]

Grant Henke has uploaded a new change for review.

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

Change subject: KUDU-2098. Drop Spark 1 Support
..

KUDU-2098. Drop Spark 1 Support

Spark 2 has been available for over a year and the value of
shipping a seperate Spark 1 artifact is decreasing.
In the case where another Spark 1 release occurs or bug fixes
are needed, updates to the Kudu Spark 1 integration can still
exist via maintenance relases.

This allows us to clean up deprecated mehods that are used for
compatibility purposes and simplifies the build.

Change-Id: I5481cc0477f4d23d89b68ef510a6c9a2aa187537
---
M build-support/jenkins/build-and-test.sh
M java/README.md
M java/gradle/dependencies.gradle
M java/kudu-spark-tools/pom.xml
M 
java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/ImportExportFiles.scala
M 
java/kudu-spark-tools/src/main/scala/org/apache/kudu/spark/tools/IntegrationTestBigLinkedList.scala
M 
java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/ITBigLinkedListTest.scala
M 
java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala
M java/kudu-spark/build.gradle
M java/kudu-spark/pom.xml
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala
R java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/package.scala
D java/kudu-spark/src/main/spark1/org/apache/kudu/spark/kudu/package.scala
M 
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
M 
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala
M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala
M java/pom.xml
18 files changed, 83 insertions(+), 243 deletions(-)


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

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

[kudu-CR] [java] Update outdated dependencies

[Jean-Daniel Cryans (Code Review)]

Jean-Daniel Cryans has posted comments on this change.

Change subject: [java] Update outdated dependencies
..


Patch Set 7: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38ea098d3c4543916c43ddb76d4e127fee93ece6
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] [java] Update outdated dependencies

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: [java] Update outdated dependencies
..


Patch Set 7: Code-Review+2

Maybe JD or Dan want to take a look too?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38ea098d3c4543916c43ddb76d4e127fee93ece6
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Grant Henke 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] separate DataDirManager from BlockManagers

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: separate DataDirManager from BlockManagers
..


Patch Set 8:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/block_manager-stress-test.cc
File src/kudu/fs/block_manager-stress-test.cc:

PS8, Line 144: 
 : 
 : 
 : 
 : 
 : 
 : 
 : 
Don't we need to preserve this in some capacity?


http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:

Line 378:   // Goes through the data dirs in 'uuid_indices' and populates
> Done. Ah interesting point; how about, "provided to the constructor"
Yeah, that's more clear.


http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:

PS8, Line 231: qcquired
acquired


http://gerrit.cloudera.org:8080/#/c/7602/8/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

Line 164
Maybe restore this comment?


Line 71:   return value == "log" || value == "file";
On macOS can you set this up so that "file" is the only valid option?


Line 270: RETURN_NOT_OK(DataDirManager::OpenExisting(env_,
Hmm, but in the case of FsManager::CreateNewFileSystemLayout followed by 
FsManager::Open, we'll end up creating the DataDirManager twice, once in 
CreateNew() and once in OpenExisting().

It's not the biggest deal I guess, but maybe it's fixable without too much work?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: separate DataDirManager from BlockManagers
..


Patch Set 7:

(11 comments)

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

Line 146: bm_.reset();
> Why does the block manager have to be destroyed separately up here? Why isn
Done


http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:

Line 289:   LOG(INFO) << "Constructing a new DataDirManager";
> Still need this?
Done


Line 292: metrics_.reset(new DataDirMetrics(std::move(opts.metric_entity)));
> Ah, I see, this is because DataDirMetrics takes the MetricEntity by cref. A
Done


Line 292: metrics_.reset(new DataDirMetrics(std::move(opts.metric_entity)));
> warning: passing result of std::move() as a const reference argument; no mo
Done


PS7, Line 339:   canonicalized_data_roots.insert(canonicalized_data_roots.end(),
 :   
canonicalized_data_roots_set.begin(),
 :   
canonicalized_data_roots_set.end());
> Isn't this logically the same as the returned values from  the calls to FsM
Not quite, it removes duplicates, but in any case, I've taken this out and 
moved it back to the FsManager.


Line 344:   dd_manager->reset(new DataDirManager(env, opts, 
canonicalized_data_roots));
> warning: parameter 'opts' is passed by value and only copied once; consider
Done


PS7, Line 350:   const int kFlagCreateTestHolePunch = 0x1;
 :   const int kFlagCreateFsync = 0x2;
> But why bother with flags at all? Flags are a means of defining an API and 
Ah, makes sense, way clearer. Done.


http://gerrit.cloudera.org:8080/#/c/7602/6/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:

PS6, Line 229:   // Shuts down all directories' thread pools.
 :   void Shutdown();
 : 
 :   // Waits on all directories' thread pools.
 :   void WaitOnClosures();
 : 
 :   // Initializes the data directories on disk.
 :   //
 :   // Returns an error if initialized directories already exist.
 :   Status Create();
 : 
 :   // Opens existing data roots from disk and indexes the files 
found.
 :   //
> Would it be possible to convert the static Init and the Create and/or Open 
Done. It was mainly a matter of ordering of canonicalization. I.e. the 
FsManager needs the canonicalized roots before doing anything; I thought I 
could push some of it to the directory manager, but I think to do a complete 
job on that might require making the WAL dir known to the directory manager too.

Originally I'd wanted to have the directory manager handle the canonicalization 
failures, but I realize we might be able to get by doing it at the FsManager 
level (e.g. by prepending an unsanitized prefix to the non-canonicalized path).

In any case, I've taken out the canonicalization changes since I don't think 
they're necessary for this patch, and probably won't be for handling failures 
at startup.


http://gerrit.cloudera.org:8080/#/c/7602/7/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:

PS7, Line 200: Defaultas to fales
> Defaults to false
Done


Line 351:   DataDirManager(Env* env,
> warning: function 'kudu::fs::DataDirManager::DataDirManager' has a definiti
Done


Line 378:   // The canonicalized roots provided at construction time, taken 
verbatim.
> I understand what this means, but "construction time" is a little weird bec
Done. Ah interesting point; how about, "provided to the constructor"


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] separate DataDirManager from BlockManagers

[Andrew Wong (Code Review)]

Hello Kudu Jenkins,

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

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

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

Change subject: separate DataDirManager from BlockManagers
..

separate DataDirManager from BlockManagers

Currently, the DataDirManager is owned by the BlockManagers. Since only
blocks are placed in 'data dirs' (i.e. subdirectories named 'data' under
the roots specified by 'fs_data_dirs'), this hierarchy made sense.
However, it would be nice to track other files that fall within
'fs_data_dirs' (e.g. tablet-metadata, consensus-metadata). Splitting the
directory manager from the block manager makes this more feasible.

This introduces a number of changes to the FsManager, BlockManager, and
DataDirManager with respect to initialization.
- BlockManagers no longer have a Create() function
- BlockManager's dtor will now just wait for DataDir closures to finish
  instead of shutting down the DataDirManager as to not directly affect
  the DataDirManager
- DataDirManagers now have two static constructors: one to open an
  existing layout, and another to create and open a new layout
- FsManagers will only create the BlockManager when opening an fs layout
- A validator is added to check the value of FLAGS_block_manager
  before any of the above initialization can occur

Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
---
M src/kudu/fs/block_manager-stress-test.cc
M src/kudu/fs/block_manager-test.cc
M src/kudu/fs/block_manager.h
M src/kudu/fs/data_dirs-test.cc
M src/kudu/fs/data_dirs.cc
M src/kudu/fs/data_dirs.h
M src/kudu/fs/file_block_manager.cc
M src/kudu/fs/file_block_manager.h
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
M src/kudu/fs/log_block_manager-test.cc
M src/kudu/fs/log_block_manager.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
16 files changed, 471 insertions(+), 325 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I387e4e88bd65298970a1e4201879de08fac07ace
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] handle disk failures during tablet copies

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: handle disk failures during tablet copies
..


Patch Set 1:

(3 comments)

I imagine Mike will do a more thorough review, but overall looks good to me.

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

Line 10: receiving data) and the copy session sources (that sending data).
"receive" and "send". Or "are receiving" and "are sending".


http://gerrit.cloudera.org:8080/#/c/7654/1/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:

PS1, Line 525:   if (group->uuid_indices().size() != 
valid_uuid_indices.size()) {
 : return Status::IOError("Directory group contains a 
failed directory");
 :   }
 :   group_uuid_indices = _uuid_indices;
Unrelated to this patch?


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

Line 305: 
Leftover from a change since removed? Or is this stylistic?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic18d93c218ea13f3086f420a4847cb5e29a47bc7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Andrew Wong 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] rpc: some small cleanup in ConnectionId

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: rpc: some small cleanup in ConnectionId
..


Patch Set 1:

(1 comment)

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

PS1, Line 68: set_real_user
nit: maybe, it's worth updating the signature of 
UserCredentials::set_real_user() to fit move semantics as well?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0788052f8c943ef102f3f551a85a8b219c65c361
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] gutil: remove use of deprecated headers

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: gutil: remove use of deprecated headers
..


Patch Set 1:

(1 comment)

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

PS1, Line 214: #if defined(__GNUC__)
 : // Use our nice hash function for strings
 : template
 : struct hash > {
 :   size_t operator()(const std::basic_string<_CharT, _Traits, 
_Alloc>& k) const {
 : return HashTo32(k.data(), static_cast(k.length()));
 :   }
 : };
 : 
 : // they don't define a hash for const string at all
 : template<> struct hash {
 :   size_t operator()(const std::string& k) const {
 : return HashTo32(k.data(), static_cast(k.length()));
 :   }
 : };
 : #endif  // defined(__GNUC__)
Does it make sense to remove this as well?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] gutil: remove use of deprecated headers

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: gutil: remove use of deprecated headers
..


Patch Set 1:

looks good to me, but clang doens't seem to agree :)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie088f77aa3f70d386a00166e783c66200fce781b
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: No