[kudu-CR] WIP: [iwyu] first pass

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: WIP: [iwyu] first pass
..


Patch Set 14:

definitely woudl be nice to get rid of some of the more common pragmas... 
here's a count from grep | uniq -c | sort -nk1

 10 #include   // IWYU pragma: keep
 15 // IWYU pragma: no_include 
 16 // IWYU pragma: no_include 
 26 #include  // IWYU pragma: keep
 27 // IWYU pragma: no_include 
 32 // IWYU pragma: no_include "kudu/util/logging.h"
 37 #include  // IWYU pragma: keep

These are all from IWYU bugs? Are they known bugs? If we are going to expect 
people to keep IWYU clean at precommit I wish it were a bit less error-prone, 
otherwise the extra time spent on trying to make it happy will outweigh any 
extra time saved with faster builds, no?

Or is the plan to just periodically (eg once a month) run this and fix any 
errors it comes up with, and hopefully those incremental fixes will be smaller 
than this initial patch?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
Gerrit-PatchSet: 14
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] WIP: [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 (#14).

Change subject: WIP: [iwyu] first pass
..

WIP: [iwyu] first pass

Updated C++ source files in accordance with include-what-you-use
recommendations:
  * 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:
  real  1m21.858s
  user  33m26.685s
  sys 2m19.831s

after:
  real  1m12.686s
  user  30m26.891s
  sys 2m7.627s

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

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 src/kudu/client/write_op.h
M src/kudu/clock/clock.h
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/clock/hybrid_clock.cc
M src/kudu/clock/hybrid_clock.h
M src/kudu/clock/logical_clock-test.cc
M src/kudu/clock/logical_clock.cc
M src/kudu/clock/logical_clock.h
M src/kudu/clock/mock_ntp.cc
M src/kudu/clock/mock_ntp.h
M src/kudu/clock/system_ntp.cc
M src/kudu/clock/system_ntp.h
M src/kudu/clock/system_unsync_time.cc
M src/kudu/clock/system_unsync_time.h
M src/kudu/codegen/code_cache.cc
M src/kudu/codegen/code_cache.h
M src/kudu/codegen/code_generator.cc
M src/kudu/codegen/code_generator.h
M 

[kudu-CR] WIP: [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 (#13).

Change subject: WIP: [iwyu] first pass
..

WIP: [iwyu] first pass

Updated C++ source files in accordance with include-what-you-use
recommendations:
  * 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:
  real  1m21.858s
  user  33m26.685s
  sys 2m19.831s

after:
  real  1m12.686s
  user  30m26.891s
  sys 2m7.627s

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

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 src/kudu/client/write_op.h
M src/kudu/clock/clock.h
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/clock/hybrid_clock.cc
M src/kudu/clock/hybrid_clock.h
M src/kudu/clock/logical_clock-test.cc
M src/kudu/clock/logical_clock.cc
M src/kudu/clock/logical_clock.h
M src/kudu/clock/mock_ntp.cc
M src/kudu/clock/mock_ntp.h
M src/kudu/clock/system_ntp.cc
M src/kudu/clock/system_ntp.h
M src/kudu/clock/system_unsync_time.cc
M src/kudu/clock/system_unsync_time.h
M src/kudu/codegen/code_cache.cc
M src/kudu/codegen/code_cache.h
M src/kudu/codegen/code_generator.cc
M src/kudu/codegen/code_generator.h
M 

[kudu-CR] WIP: [iwyu] first pass

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: WIP: [iwyu] first pass
..


Patch Set 12:

(21 comments)

http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/rle.cc
File src/kudu/benchmarks/rle.cc:

Line 28: #include 
> Might be good to make a comment in the commit message about these different
Good idea.  I hope keeping these pragmas is a temporary solution until some 
bugs in the include-what-you-use tool are fixed or we find some other way of 
fixing it.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/line_item_tsv_importer.h
File src/kudu/benchmarks/tpch/line_item_tsv_importer.h:

Line 21: #include 
> leftover?
Done


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc
File src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc:

Line 18: #include 
> sys/types.h and gutil/port.h seem suspicious to me.  I don't see anything i
Yep, that's one of those recommendation the IWYU tools gave.  As I already 
mentioned -- it's necessary to validate its suggestions for every time, and 
that takes a lot of time.

Here it suggested to include  for int64_t.  Apparently, it should 
have been  or  instead.


Line 18: #include 
> Perhaps sys/types.h is for the int64_t type?
Right.  Now it's fixed.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/rpc_line_item_dao.h
File src/kudu/benchmarks/tpch/rpc_line_item_dao.h:

Line 24: #include "kudu/client/client.h"
> Could you forward declare boost::function instead?
Good idea.  However, IWYU suggested this:

kudu/src/kudu/benchmarks/tpch/rpc_line_item_dao.h should add these lines:
#include   // for function
namespace boost { template  class function; }

Adding only the forward declaration would be enough.  But if so, then IWYU 
complained continued to complain and I decided to just add .

Also, it seems the previous version of the IWYU tools gave a different 
suggestions, not matching with those that current does (current is of v0.8).

I'll update this to keep the forward declaration and adding a pragma for not 
suggesting to include 


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/tpch-schemas.h
File src/kudu/benchmarks/tpch/tpch-schemas.h:

Line 25: #include "kudu/client/schema.h"
> Perhaps we could add this include inside the '#ifdef KUDU_HEADERS_NO_STUBS'
Yep, this is kind of strange left-over from my earlier experiments, and it 
seems I forgot to address it.

Thank you for pointing at it this time as well -- I'll address this.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/tpch1.cc
File src/kudu/benchmarks/tpch/tpch1.cc:

Line 60: #include 
> Another un-obvious one.
That was a suggestion from IWYU for int32_t.  Apparently, it should be covered 
by .  I'll check what the current 0.8 version suggests.


Line 60: #include 
> Probably int32_t (see struct Result).
yep, that's true


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/client/scan_predicate.cc
File src/kudu/client/scan_predicate.cc:

Line 48: 
> extra
Done


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

Line 24: #include 
> Hm, did plain boost/optional.hpp result in a warning?
Yes, it did.  That's why I updated this.  We can amend this using our own 
mappings for the boost library.  Right now I'm using the 'standard' mappings 
from the IWYU source tree.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/client/table_creator-internal.h
File src/kudu/client/table_creator-internal.h:

Line 24: #include 
> I think this could be optional_fwd.hpp
The class has a member of boost::optional type, so I don't think forward 
declaration would be feasible here.

Yep -- the compiler output an error if trying to use the forward declaration.

IWYU tool suggested to use , which seems good enough.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/client/write_op.h
File src/kudu/client/write_op.h:

Line 20: // NOTE: using stdint.h instead of cstdint because this file is 
supposed
> client_samples-test covers this, right?  Probably no need to comment this a
Done


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/clock/logical_clock.cc
File src/kudu/clock/logical_clock.cc:

Line 73: const MonoTime&) {
> Don't we usually do something like /* deadline */?
Done


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/codegen/row_projector.h
File src/kudu/codegen/row_projector.h:

Line 35: #include "kudu/gutil/ref_counted.h"
> These keep showing up over and over, does that indicate some kind of issue 
I think this manifests a bug in the include-what-you-use tool, not with our 
header files organization.

It's more apparent when you see the suggestions that IWYU outputs:

/row_projector.h should add these lines:
...
#include "kudu/gutil/int128.h" // for ostream

[kudu-CR] WIP: [iwyu] first pass

[Alexey Serbin (Code Review)]

Alexey Serbin has uploaded a new patch set (#12).

Change subject: WIP: [iwyu] first pass
..

WIP: [iwyu] first pass

Updated C++ source files in accordance with include-what-you-use
recommendations:
  * 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:
  real  1m21.858s
  user  33m26.685s
  sys 2m19.831s

after:
  real  1m12.686s
  user  30m26.891s
  sys 2m7.627s

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

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 src/kudu/client/write_op.h
M src/kudu/clock/clock.h
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/clock/hybrid_clock.cc
M src/kudu/clock/hybrid_clock.h
M src/kudu/clock/logical_clock-test.cc
M src/kudu/clock/logical_clock.cc
M src/kudu/clock/logical_clock.h
M src/kudu/clock/mock_ntp.cc
M src/kudu/clock/mock_ntp.h
M src/kudu/clock/system_ntp.cc
M src/kudu/clock/system_ntp.h
M src/kudu/clock/system_unsync_time.cc
M src/kudu/clock/system_unsync_time.h
M src/kudu/codegen/code_cache.cc
M src/kudu/codegen/code_cache.h
M src/kudu/codegen/code_generator.cc
M src/kudu/codegen/code_generator.h
M src/kudu/codegen/codegen-test.cc
M src/kudu/codegen/compilation_manager.cc
M src/kudu/codegen/compilation_manager.h
M 

[kudu-CR] WIP: [iwyu] first pass

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: WIP: [iwyu] first pass
..


Patch Set 11:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc
File src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc:

Line 18: #include 
> sys/types.h and gutil/port.h seem suspicious to me.  I don't see anything i
Perhaps sys/types.h is for the int64_t type?


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/tpch1.cc
File src/kudu/benchmarks/tpch/tpch1.cc:

Line 60: #include 
> Another un-obvious one.
Probably int32_t (see struct Result).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: [iwyu] first pass

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: WIP: [iwyu] first pass
..


Patch Set 11: -Verified

> (19 comments)
 > 
 > Thanks for banging on this some more, will be great to have with
 > the automated checking.  I only got through about 40%, but I saw a
 > lot of repeated patterns that are kind of suspicious.  Basically
 > most uses of the pragmas surprised me.  Are these pragmas
 > indicating issues with IWYU, or with our includes?  In particular
 > logging.h, port.h, and boost headers keep reappearing.

Thank you for taking a look at this.

The automated checking is coming -- I added the script to mute warnings for 
some files in a separate changelist and I'm planning to include it into a 
pre-commit build.  I just was hoping to remove as many files from the mute list 
as possible for now.

Yes, those pragmas manifest issues with IWYU -- it's still in alpha quality, if 
not lower.  The pattern with  is a re-occurring one, and there 
are many more like that.  And there are even worse ones, which breaks 
compilation.  The problem is that I don't know how to automate the process of 
straighten it up, and it requires a lot of manual intervention (that's why so 
much time is spent on that already).

The only hope is to try mappings for the boost headers -- that type of 
suggestions might be straighten up as documented.  As for  -- I 
tried a mapping with previous version of the IWYU tool, but no luck.

I'll add a link about the pragmas into the commit message, sure.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] WIP: [iwyu] first pass

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: WIP: [iwyu] first pass
..


Patch Set 11:

(19 comments)

Thanks for banging on this some more, will be great to have with the automated 
checking.  I only got through about 40%, but I saw a lot of repeated patterns 
that are kind of suspicious.  Basically most uses of the pragmas surprised me.  
Are these pragmas indicating issues with IWYU, or with our includes?  In 
particular logging.h, port.h, and boost headers keep reappearing.

http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/rle.cc
File src/kudu/benchmarks/rle.cc:

Line 28: #include  // IWYU pragma: keep
Might be good to make a comment in the commit message about these different 
pragma types, e.g. what are they doing, and is it a long term or temporary 
solution?


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/line_item_tsv_importer.h
File src/kudu/benchmarks/tpch/line_item_tsv_importer.h:

Line 21: //#include 
leftover?


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc
File src/kudu/benchmarks/tpch/rpc_line_item_dao-test.cc:

Line 18: #include 
sys/types.h and gutil/port.h seem suspicious to me.  I don't see anything in 
this file which might need those.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/rpc_line_item_dao.h
File src/kudu/benchmarks/tpch/rpc_line_item_dao.h:

Line 24: #include  // IWYU pragma: keep
Could you forward declare boost::function instead?


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/tpch-schemas.h
File src/kudu/benchmarks/tpch/tpch-schemas.h:

Line 25: #include  // for CHECK_OK to be picked up in status.h
Perhaps we could add this include inside the '#ifdef KUDU_HEADERS_NO_STUBS' in 
status.h instead of including it in all the call sites?  E.g. right here: 
https://github.com/apache/kudu/blob/d1f53cc323d5d07e79304765fe171f535c1d548a/src/kudu/util/status.h#L22


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/benchmarks/tpch/tpch1.cc
File src/kudu/benchmarks/tpch/tpch1.cc:

Line 60: #include 
Another un-obvious one.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/client/scan_predicate.cc
File src/kudu/client/scan_predicate.cc:

Line 48: 
extra


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

Line 24: #include 
Hm, did plain boost/optional.hpp result in a warning?


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/client/table_creator-internal.h
File src/kudu/client/table_creator-internal.h:

Line 24: #include  // IWYU pragma: keep
I think this could be optional_fwd.hpp


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/client/write_op.h
File src/kudu/client/write_op.h:

Line 20: // NOTE: using stdint.h instead of cstdint because this file is 
supposed
client_samples-test covers this, right?  Probably no need to comment this and 
client.h if we do.  These headers are meant to be consumed externally and I 
think it may be a bit confusing without context.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/clock/logical_clock.cc
File src/kudu/clock/logical_clock.cc:

Line 73: const MonoTime&) {
Don't we usually do something like /* deadline */?


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/codegen/row_projector.h
File src/kudu/codegen/row_projector.h:

Line 35: // IWYU pragma: no_include "kudu/gutil/int128.h"
These keep showing up over and over, does that indicate some kind of issue with 
our organization?


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/common/column_predicate.cc
File src/kudu/common/column_predicate.cc:

Line 34: 
extra


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/common/partition_pruner.h
File src/kudu/common/partition_pruner.h:

Line 31: class Partition;// IWYU pragma: keep
I think these forward declares, or perhaps the include of partition.h could be 
removed.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/common/row_changelist.cc
File src/kudu/common/row_changelist.cc:

Line 31: #include "kudu/gutil/port.h"
This one keeps showing up, not sure what it's being used for.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/common/row_changelist.h
File src/kudu/common/row_changelist.h:

Line 41: class faststring; // IWYU pragma: keep
I Don't see why the pragma would be necessary with the fwd declare?


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/common/scan_spec.cc
File src/kudu/common/scan_spec.cc:

Line 41: // IWYU pragma: no_include 
These are the only two uses of boost in the file, that's suspicious.


http://gerrit.cloudera.org:8080/#/c/4738/11/src/kudu/common/schema-test.cc
File src/kudu/common/schema-test.cc:

Line 18: 
I think it's in our style to include the tested header first.

[kudu-CR] WIP: [iwyu] first pass

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: WIP: [iwyu] first pass
..


Patch Set 11: Verified+1

unrelated flake in raft_consensus-itest (TSAN build only)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] WIP: [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 (#11).

Change subject: WIP: [iwyu] first pass
..

WIP: [iwyu] first pass

Updated C++ source files in accordance with include-what-you-use
recommendations:
  * 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:
  real  1m21.858s
  user  33m26.685s
  sys 2m19.831s

after:
  real  1m12.686s
  user  30m26.891s
  sys 2m7.627s

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

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/tpch-schemas.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-base.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.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/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 src/kudu/client/write_op.h
M src/kudu/clock/clock.h
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/clock/hybrid_clock.cc
M src/kudu/clock/hybrid_clock.h
M src/kudu/clock/logical_clock-test.cc
M src/kudu/clock/logical_clock.cc
M src/kudu/clock/logical_clock.h
M src/kudu/codegen/code_cache.cc
M src/kudu/codegen/code_cache.h
M src/kudu/codegen/code_generator.cc
M src/kudu/codegen/code_generator.h
M src/kudu/codegen/codegen-test.cc
M src/kudu/codegen/compilation_manager.cc
M src/kudu/codegen/compilation_manager.h
M src/kudu/codegen/jit_wrapper.cc
M src/kudu/codegen/jit_wrapper.h
M src/kudu/codegen/module_builder.cc

[kudu-CR] WIP: [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 (#10).

Change subject: WIP: [iwyu] first pass
..

WIP: [iwyu] first pass

Updated C++ source files in accordance with include-what-you-use
recommendations:
  * 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:
  real  1m21.858s
  user  33m26.685s
  sys 2m19.831s

after:
  real  1m12.686s
  user  30m26.891s
  sys 2m7.627s

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

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/tpch-schemas.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-base.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.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/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 src/kudu/client/write_op.h
M src/kudu/clock/clock.h
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/clock/hybrid_clock.cc
M src/kudu/clock/hybrid_clock.h
M src/kudu/clock/logical_clock-test.cc
M src/kudu/clock/logical_clock.cc
M src/kudu/clock/logical_clock.h
M src/kudu/codegen/code_cache.cc
M src/kudu/codegen/code_cache.h
M src/kudu/codegen/code_generator.cc
M src/kudu/codegen/code_generator.h
M src/kudu/codegen/codegen-test.cc
M src/kudu/codegen/compilation_manager.cc
M src/kudu/codegen/compilation_manager.h
M src/kudu/codegen/jit_wrapper.cc
M src/kudu/codegen/jit_wrapper.h
M src/kudu/codegen/module_builder.cc

[kudu-CR] WIP: [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 (#8).

Change subject: WIP: [iwyu] first pass
..

WIP: [iwyu] first pass

Updated C++ source files in accordance with include-what-you-use
recommendations:
  * 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:
  real  1m21.858s
  user  33m26.685s
  sys 2m19.831s

after:
  real  1m12.686s
  user  30m26.891s
  sys 2m7.627s

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

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/tpch-schemas.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-base.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.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/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 src/kudu/client/write_op.h
M src/kudu/clock/clock.h
M src/kudu/clock/hybrid_clock-test.cc
M src/kudu/clock/hybrid_clock.cc
M src/kudu/clock/hybrid_clock.h
M src/kudu/clock/logical_clock-test.cc
M src/kudu/clock/logical_clock.cc
M src/kudu/clock/logical_clock.h
M src/kudu/codegen/code_cache.cc
M src/kudu/codegen/code_cache.h
M src/kudu/codegen/code_generator.cc
M src/kudu/codegen/code_generator.h
M src/kudu/codegen/codegen-test.cc
M src/kudu/codegen/compilation_manager.cc
M src/kudu/codegen/compilation_manager.h
M src/kudu/codegen/jit_wrapper.cc
M src/kudu/codegen/jit_wrapper.h
M src/kudu/codegen/module_builder.cc
M 

[kudu-CR] WIP: [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 (#7).

Change subject: WIP: [iwyu] first pass
..

WIP: [iwyu] first pass

Updated C++ source files in accordance with include-what-you-use
recommendations:
  * 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:
  real  1m21.858s
  user  33m26.685s
  sys 2m19.831s

after:
  real  1m12.686s
  user  30m26.891s
  sys 2m7.627s

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

Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
---
M src/kudu/benchmarks/rle.cc
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/tpch-schemas.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-base.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/compression-test.cc
M src/kudu/cfile/compression_codec.cc
M src/kudu/cfile/compression_codec.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/gvint_block.cc
M src/kudu/cfile/gvint_block.h
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.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.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 src/kudu/client/write_op.h
M src/kudu/codegen/code_cache.cc
M src/kudu/codegen/code_cache.h
M src/kudu/codegen/code_generator.cc
M src/kudu/codegen/code_generator.h
M src/kudu/codegen/codegen-test.cc
M src/kudu/codegen/compilation_manager.cc
M src/kudu/codegen/compilation_manager.h
M src/kudu/codegen/jit_wrapper.cc
M src/kudu/codegen/jit_wrapper.h
M src/kudu/codegen/module_builder.cc
M src/kudu/codegen/module_builder.h
M src/kudu/codegen/precompiled.cc
M src/kudu/codegen/row_projector.cc
M src/kudu/codegen/row_projector.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc

[kudu-CR] WIP: [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 (#6).

Change subject: WIP: [iwyu] first pass
..

WIP: [iwyu] first pass

Updated C++ source files in accordance with include-what-you-use
recommendations:
  * 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:
  real  1m21.858s
  user  33m26.685s
  sys 2m19.831s

after:
  real  1m12.686s
  user  30m26.891s
  sys 2m7.627s

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

Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
---
M src/kudu/benchmarks/rle.cc
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/tpch-schemas.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-base.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/compression-test.cc
M src/kudu/cfile/compression_codec.cc
M src/kudu/cfile/compression_codec.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/gvint_block.cc
M src/kudu/cfile/gvint_block.h
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.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.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/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 src/kudu/client/write_op.h
M src/kudu/codegen/code_cache.cc
M src/kudu/codegen/code_cache.h
M src/kudu/codegen/code_generator.cc
M src/kudu/codegen/code_generator.h
M src/kudu/codegen/codegen-test.cc
M src/kudu/codegen/compilation_manager.cc
M src/kudu/codegen/compilation_manager.h
M src/kudu/codegen/jit_wrapper.cc
M src/kudu/codegen/jit_wrapper.h
M src/kudu/codegen/module_builder.cc
M src/kudu/codegen/module_builder.h
M src/kudu/codegen/precompiled.cc
M src/kudu/codegen/row_projector.cc
M src/kudu/codegen/row_projector.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h

[kudu-CR] WIP: [iwyu] first pass

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: WIP: [iwyu] first pass
..


Patch Set 5:

> > Also, the codegen test is broken due to some unexpected reason. 
 > I
 > > need to figure what went wrong with that.  But if you are
 > familiar
 > > with that part and can guess what's the problem (it could save me
 > > some time which I would appreciate a lot), see below for the
 > error:
 > >
 > > F1128 19:14:14.026911 2025766912 codegen-test.cc:257] Check
 > failed:
 > > _s.ok() Bad status: Configuration error: Code generation for
 > module
 > > failed. Could not start ExecutionEngine: Interpreter has not been
 > > linked in.
 > 
 > I reproed the codegen-test failure with your patch, then fixed it
 > with this. I was following 
 > http://comments.gmane.org/gmane.comp.compilers.llvm.devel/42986
 > as a guide. That header is...something wacky. Maybe we're missing
 > some cmake logic that would obviate the need for directly including
 > MCJIT.h?
 > 
 > diff --git a/src/kudu/codegen/module_builder.cc 
 > b/src/kudu/codegen/module_builder.cc
 > index f4d858e..7634577 100644
 > --- a/src/kudu/codegen/module_builder.cc
 > +++ b/src/kudu/codegen/module_builder.cc
 > @@ -28,6 +28,7 @@
 > #include 
 > #include 
 > #include 
 > +#include 
 > #include 
 > #include 
 > #include 

Wow, that's great!  Thanks, Adar!
Much appreciated!

I'll include your patch in the next revision of this patch.  I hope with that 
and small other fixes the tests should pass.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
Gerrit-PatchSet: 5
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] WIP: [iwyu] first pass

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: WIP: [iwyu] first pass
..


Patch Set 5:

> Also, the codegen test is broken due to some unexpected reason.  I
 > need to figure what went wrong with that.  But if you are familiar
 > with that part and can guess what's the problem (it could save me
 > some time which I would appreciate a lot), see below for the error:
 > 
 > F1128 19:14:14.026911 2025766912 codegen-test.cc:257] Check failed:
 > _s.ok() Bad status: Configuration error: Code generation for module
 > failed. Could not start ExecutionEngine: Interpreter has not been
 > linked in.

I reproed the codegen-test failure with your patch, then fixed it with this. I 
was following http://comments.gmane.org/gmane.comp.compilers.llvm.devel/42986 
as a guide. That header is...something wacky. Maybe we're missing some cmake 
logic that would obviate the need for directly including MCJIT.h?

  diff --git a/src/kudu/codegen/module_builder.cc 
b/src/kudu/codegen/module_builder.cc
  index f4d858e..7634577 100644
  --- a/src/kudu/codegen/module_builder.cc
  +++ b/src/kudu/codegen/module_builder.cc
  @@ -28,6 +28,7 @@
   #include 
   #include 
   #include 
  +#include 
   #include 
   #include 
   #include 

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
Gerrit-PatchSet: 5
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] WIP: [iwyu] first pass

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: WIP: [iwyu] first pass
..


Patch Set 5:

(3 comments)

> (3 comments)
 > 
 > Just did a scan through, mostly looked at fs/ and tserver/ (for
 > fun).
 > 
 > One thing comes to mind: this will bitrot very quickly if IWYU
 > doesn't run precommit (as part of gerrit) to flag bad behavior. How
 > realistic do you think that is right now?

Yes, I agree -- the changes in header files are very common, and it took me a 
while to keep up with the flow of incoming patches even for this single patch.

As for the automating the IWYU checks, I think it's possible to take care of 
that over the coming weekend (formally, I'm not allowed to spend my time on 
this since the consistent operations stuff is the higher priority thing right 
now).

I have part of the missing pieces -- the patch to fetch and build IWYU with the 
llvm suite (need to update to the newer IWYU version which appeared in October).

But I need to run the IWYU in many passes with the map files I built and make 
sure no more suggestions appear.  Also, I need to disregard the auto-gen 
protobuf stuff (will use awk for output parsing, so that sounds easy to do).

Also, the codegen test is broken due to some unexpected reason.  I need to 
figure what went wrong with that.  But if you are familiar with that part and 
can guess what's the problem (it could save me some time which I would 
appreciate a lot), see below for the error:

F1128 19:14:14.026911 2025766912 codegen-test.cc:257] Check failed: _s.ok() Bad 
status: Configuration error: Code generation for module failed. Could not start 
ExecutionEngine: Interpreter has not been linked in.

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

Line 38: DECLARE_bool(block_coalesce_close);
> I wonder why this is here. I can't see a good reason.
Good catch -- it seems we can remove this declaration along with 
 header


http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/tserver/scanners-test.cc
File src/kudu/tserver/scanners-test.cc:

Line 30: #include 
> Nit: this belongs before the gutil includes.
yep -- it seems I did a vim command typo while working with those text blocks.  
I also missed the  header.

Will fix, thanks.


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

Line 25: #include 
> Nit: belongs in the same grouping as gflags/glog/protobuf.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
Gerrit-PatchSet: 5
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: [iwyu] first pass

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: WIP: [iwyu] first pass
..


Patch Set 5:

(3 comments)

Just did a scan through, mostly looked at fs/ and tserver/ (for fun).

One thing comes to mind: this will bitrot very quickly if IWYU doesn't run 
precommit (as part of gerrit) to flag bad behavior. How realistic do you think 
that is right now?

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

Line 38: DECLARE_bool(block_coalesce_close);
I wonder why this is here. I can't see a good reason.


http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/tserver/scanners-test.cc
File src/kudu/tserver/scanners-test.cc:

Line 30: #include 
Nit: this belongs before the gutil includes.


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

Line 25: #include 
Nit: belongs in the same grouping as gflags/glog/protobuf.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
Gerrit-PatchSet: 5
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: [iwyu] first pass

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: WIP: [iwyu] first pass
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:

Line 50: #include "kudu/gutil/int128.h"
> ah, ok.  So this is necessary for logging the block IDs?  I didn't realize 
Clarification: I think the C++ includes work the way you expect them to work.  
However, the issue with IWYU here is that it wanted std::ostream forward 
declaration from int128.h somehow, regardless requiring inclusion of  
as well.  That I need to troubleshoot and fix.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
Gerrit-PatchSet: 5
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: [iwyu] first pass

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: WIP: [iwyu] first pass
..


Patch Set 5:

(4 comments)

> (5 comments)
 > 
 > Only got through about 20% of the files, but it looks like there
 > are some cases where headers are getting included unnecessarily (I
 > noticed a few cases of glog/logging.h and int128.h).  Is this down
 > to peculiarities with IWYU, or misconfiguration, or something else?

Thank you for the review!

The IWYU tool in default configuration does not honor transitive inclusion.  
That's the reason we see so many of them added.

I've chosen the non-transitive configuration because I was actively tossing the 
includes back and forth -- it would be harder working in transitive case here 
(i.e. more unexpected breakages while in progress).  It has stabilized now and 
I'm thinking about moving into the transitive mode once cleaning this up a bit 
and having IWYU mappings ready.

As for that int128.h, I responded for the particular case in-line.

http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/benchmarks/tpch/rpc_line_item_dao.h
File src/kudu/benchmarks/tpch/rpc_line_item_dao.h:

Line 24: #include  // IWYU pragma: keep
> Is this due to a bug in IWYU?  Could boost/function_fwd.hpp help?
That's more about the IWYU's nature -- it wants you to include the very leaf 
file that contains the necessary definitions ('leaf' in the terms of the header 
include chain).

E.g., instead of  it suggests to include

#include 

In case of bind.hpp or alike it might be worse.

The idea to deal with that: create a set of mapping files which would allow 
IWYU to include 'interface-like' headers.  I'm in progress of building those 
mappings, etc.  If you are interested in details, you could take a look at:

https://github.com/include-what-you-use/include-what-you-use/blob/master/docs/IWYUMappings.md


http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/benchmarks/tpch/tpch-schemas.h
File src/kudu/benchmarks/tpch/tpch-schemas.h:

Line 25: #include  // for CHECK_OK to be picked up in status.h
> Why not include glog/logging.h in status.h?
ok, I will clarify on that.


http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/cfile/binary_prefix_block.h
File src/kudu/cfile/binary_prefix_block.h:

Line 26: #include 
> It looks like glog/logging.h is getting included in a bunch of places, I pr
Here it is for DCHECK()


http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:

Line 50: #include "kudu/gutil/int128.h"
> This is included here and a few other files, but from a quick glance it doe
Good catch!

Sometimes it's for uint128, sometimes it's for Uint128High64, sometimes it's 
for ostream (sic!)

Here it's for ostream :)

I'll need to make an additional run to check for that obscure things.

Also, IWYU in default configuration does not honor transitive inclusion.  
That's the reason we see so many of them added.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
Gerrit-PatchSet: 5
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] WIP: [iwyu] first pass

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: WIP: [iwyu] first pass
..


Patch Set 5:

(5 comments)

Only got through about 20% of the files, but it looks like there are some cases 
where headers are getting included unnecessarily (I noticed a few cases of 
glog/logging.h and int128.h).  Is this down to peculiarities with IWYU, or 
misconfiguration, or something else?

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

Line 15: As a result, time of compilation reduced ~10% if building with GNU make
Nice!


http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/benchmarks/tpch/rpc_line_item_dao.h
File src/kudu/benchmarks/tpch/rpc_line_item_dao.h:

Line 24: #include  // IWYU pragma: keep
Is this due to a bug in IWYU?  Could boost/function_fwd.hpp help?


http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/benchmarks/tpch/tpch-schemas.h
File src/kudu/benchmarks/tpch/tpch-schemas.h:

Line 25: #include  // for CHECK_OK to be picked up in status.h
Why not include glog/logging.h in status.h?


http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/cfile/binary_prefix_block.h
File src/kudu/cfile/binary_prefix_block.h:

Line 26: #include 
It looks like glog/logging.h is getting included in a bunch of places, I 
presume just for the hidden dependency in status.h?  That seems antithetical to 
IWYU.


http://gerrit.cloudera.org:8080/#/c/4738/5/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:

Line 50: #include "kudu/gutil/int128.h"
This is included here and a few other files, but from a quick glance it doesn't 
seem necessary.


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

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

[kudu-CR] WIP: [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 (#5).

Change subject: WIP: [iwyu] first pass
..

WIP: [iwyu] first pass

Updated C++ source files in accordance with include-what-you-use
recommendations:
  * 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:
  real  1m21.858s
  user  33m26.685s
  sys 2m19.831s

after:
  real  1m12.686s
  user  30m26.891s
  sys 2m7.627s

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

Change-Id: I6923091be36a1c03fac34dd73e6b17e17ac9ceaa
---
M src/kudu/benchmarks/rle.cc
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/tpch-schemas.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-base.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/compression-test.cc
M src/kudu/cfile/compression_codec.cc
M src/kudu/cfile/compression_codec.h
M src/kudu/cfile/encoding-test.cc
M src/kudu/cfile/gvint_block.cc
M src/kudu/cfile/gvint_block.h
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.h
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.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/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 src/kudu/client/write_op.h
M src/kudu/codegen/code_cache.cc
M src/kudu/codegen/code_cache.h
M src/kudu/codegen/code_generator.cc
M src/kudu/codegen/code_generator.h
M src/kudu/codegen/codegen-test.cc
M src/kudu/codegen/compilation_manager.cc
M src/kudu/codegen/compilation_manager.h
M src/kudu/codegen/jit_wrapper.cc
M src/kudu/codegen/jit_wrapper.h
M src/kudu/codegen/module_builder.cc
M src/kudu/codegen/module_builder.h
M src/kudu/codegen/precompiled.cc
M src/kudu/codegen/row_projector.cc
M src/kudu/codegen/row_projector.h
M src/kudu/common/column_predicate-test.cc
M src/kudu/common/column_predicate.cc
M src/kudu/common/column_predicate.h
M