You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2016/11/19 00:55:28 UTC

[kudu-CR] block manager: start using the file cache 

Hello Dan Burkert, Todd Lipcon,

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

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

to review the following change.

Change subject: block manager: start using the file cache
......................................................................

block manager: start using the file cache

This commit integrates the file cache into both the file and log block
managers. The capacity of the cache is determined at runtime by inspecting
the RLIMIT_NOFILE resource limit; because the cache doesn't manage all open
files, we use a rather conservative 50% of the limit. This can be overridden
with a command line flag.

Other changes of note:
- Unlike the FBM, the LBM creates and opens container files for read/write
  in the same operation. Since that kind of behavior isn't supported by the
  file cache, we close the files after creating them, then reopen them
  through the cache. While inelegant, I don't expect this to be problematic.
- block_manager-stress-test now uses a PeriodicOpenFdChecker to make sure
  the file cache is working correctly. Some of the test behavior was tweaked
  to increase the number of blocks.
- BlockManagerTest.CloseManyBlocksTest now uses 1000 blocks, which is enough
  for it to fail with the FBM sans file cache when the process resource
  limit is 1024 open files (the default on my Ubuntu 16.04 installation).

Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
---
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.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
13 files changed, 150 insertions(+), 86 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] block manager: start using the file cache

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: block manager: start using the file cache
......................................................................

block manager: start using the file cache

This commit integrates the file cache into both the file and log block
managers. The capacity of the cache is determined at runtime by inspecting
the RLIMIT_NOFILE resource limit; because the cache doesn't manage all open
files, we use a rather conservative 50% of the limit. This can be overridden
with a command line flag.

Other changes of note:
- Unlike the FBM, the LBM creates and opens container files for read/write
  in the same operation. Since that kind of behavior isn't supported by the
  file cache, we close the files after creating them, then reopen them
  through the cache. While inelegant, I don't expect this to be problematic.
- block_manager-stress-test now uses a PeriodicOpenFdChecker to make sure
  the file cache is working correctly. Some of the test behavior was tweaked
  to increase the number of blocks.
- BlockManagerTest.CloseManyBlocksTest now uses 1000 blocks, which is enough
  for it to fail with the FBM sans file cache when the process resource
  limit is 1024 open files (the default on my Ubuntu 16.04 installation).
- When constructing a block manager, we first try to increase the
  RLIMIT_NOFILE soft limit for the process to be equal to the hard limit. On
  many systems there's enough of a gap between them that this can add a lot
  of cache capacity "for free".

Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
---
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.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
13 files changed, 193 insertions(+), 88 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] block manager: start using the file cache

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: block manager: start using the file cache
......................................................................

block manager: start using the file cache

This commit integrates the file cache into both the file and log block
managers. The capacity of the cache is determined at runtime by inspecting
the RLIMIT_NOFILE resource limit; because the cache doesn't manage all open
files, we use a rather conservative 50% of the limit. This can be overridden
with a command line flag.

Other changes of note:
- Unlike the FBM, the LBM creates and opens container files for read/write
  in the same operation. Since that kind of behavior isn't supported by the
  file cache, we close the files after creating them, then reopen them
  through the cache. While inelegant, I don't expect this to be problematic.
- block_manager-stress-test now uses a PeriodicOpenFdChecker to make sure
  the file cache is working correctly. Some of the test behavior was tweaked
  to increase the number of blocks, and to avoid violating the semantics of
  the file cache.
- BlockManagerTest.CloseManyBlocksTest now uses 1000 blocks, which is enough
  for it to fail with the FBM sans file cache when the process resource
  limit is 1024 open files (the default on my Ubuntu 16.04 installation).
- When constructing a block manager, we first try to increase the
  RLIMIT_NOFILE soft limit for the process to be equal to the hard limit. On
  many systems there's enough of a gap between them that this can add a lot
  of cache capacity "for free".

Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
---
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.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
13 files changed, 251 insertions(+), 115 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] block manager: start using the file cache

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: block manager: start using the file cache
......................................................................


Patch Set 4:

> * Make sure this handles the case where the ulimit is unlimited

As we discovered, it doesn't appear possible to set RLIMIT_NOFILE to RLIMIT_INFINITY.

To maximize RLIMIT_NOFILE, an admin has to do the following:
1. Maximize /proc/sys/fs/nr_open. On my Ubuntu 16.04 system the max value is 2147483584.
2. Maximize /proc/sys/fs/file-max. On my system the max value can be -1 (i.e. 18446744073709551615).
3. Call setrlimit(RLIMIT_NOFILE, val). My kernel lets me go as high as /proc/sys/fs/nr_open.

Given all of this, I think the current behavior is still correct: use half of the available limit.

Separately, I've added code to jack up the soft limit to be equal to the hard limit. I've incorporated it into this patch.

 > * Consider adding a mandatory minimum ulimit with an unsafe escape
 > hatch flag in order to avoid situations where Kudu thrashes on an
 > extremely low number of files

Okay, though I think I'll rely on benchmark results to determine this minium.

 > * It would be nice to see a benchmark comparing number of file
 > descriptors vs. performance on something like YCSB (which is a
 > workload I would expect would be dependent on accessing many
 > blocks)

Okay.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] block manager: start using the file cache

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has uploaded a new patch set (#3).

Change subject: block manager: start using the file cache
......................................................................

block manager: start using the file cache

This commit integrates the file cache into both the file and log block
managers. The capacity of the cache is determined at runtime by inspecting
the RLIMIT_NOFILE resource limit; because the cache doesn't manage all open
files, we use a rather conservative 50% of the limit. This can be overridden
with a command line flag.

Other changes of note:
- Unlike the FBM, the LBM creates and opens container files for read/write
  in the same operation. Since that kind of behavior isn't supported by the
  file cache, we close the files after creating them, then reopen them
  through the cache. While inelegant, I don't expect this to be problematic.
- block_manager-stress-test now uses a PeriodicOpenFdChecker to make sure
  the file cache is working correctly. Some of the test behavior was tweaked
  to increase the number of blocks.
- BlockManagerTest.CloseManyBlocksTest now uses 1000 blocks, which is enough
  for it to fail with the FBM sans file cache when the process resource
  limit is 1024 open files (the default on my Ubuntu 16.04 installation).

Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
---
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.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
13 files changed, 150 insertions(+), 87 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] block manager: start using the file cache

Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.

Change subject: block manager: start using the file cache
......................................................................


Patch Set 4:

(1 comment)

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

PS4, Line 366: OpenExistingRWFile
I am curious to know what's the challenge in driving the new file creation through the FileCache(assuming we add a support for that in FileCache) @L350 instead of creating first and then opening through cache. I saw the COMMIT_MSG mentioning it too but didn't understand the reasons behind it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] block manager: start using the file cache

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: block manager: start using the file cache
......................................................................


Patch Set 10: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] block manager: start using the file cache

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: block manager: start using the file cache
......................................................................

block manager: start using the file cache

This commit integrates the file cache into both the file and log block
managers. The capacity of the cache is determined at runtime by inspecting
the RLIMIT_NOFILE resource limit; because the cache doesn't manage all open
files, we use a rather conservative 50% of the limit. This can be overridden
with a command line flag.

Other changes of note:
- Unlike the FBM, the LBM creates and opens container files for read/write
  in the same operation. Since that kind of behavior isn't supported by the
  file cache, we close the files after creating them, then reopen them
  through the cache. While inelegant, I don't expect this to be problematic.
- block_manager-stress-test now uses a PeriodicOpenFdChecker to make sure
  the file cache is working correctly. Some of the test behavior was tweaked
  to increase the number of blocks.
- BlockManagerTest.CloseManyBlocksTest now uses 1000 blocks, which is enough
  for it to fail with the FBM sans file cache when the process resource
  limit is 1024 open files (the default on my Ubuntu 16.04 installation).
- When constructing a block manager, we first try to increase the
  RLIMIT_NOFILE soft limit for the process to be equal to the hard limit. On
  many systems there's enough of a gap between them that this can add a lot
  of cache capacity "for free".

Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
---
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.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
13 files changed, 191 insertions(+), 88 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] block manager: start using the file cache

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: block manager: start using the file cache
......................................................................


Patch Set 8:

(2 comments)

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

Line 61:     static std::once_flag once;
> So, WARN if the gflag value is higher than GetOpenFileLimit()? Yeah, I can 
I think WARN at a minimum, I was suggesting FATAL, since it seems like a time bomb waiting to go off at peak load.


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

Line 43: TAG_FLAG(block_manager_max_open_files, advanced);
Should we mark this as unstable/evolving?  I can imagine we would want to revert to always using the file cache once we prove it's stable.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] block manager: start using the file cache

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: block manager: start using the file cache
......................................................................


Patch Set 3:

code looks good, going to jot down some notes we discussed offline:

* Make sure this handles the case where the ulimit is unlimited
* Consider adding a mandatory minimum ulimit with an unsafe escape hatch flag in order to avoid situations where Kudu thrashes on an extremely low number of files
* It would be nice to see a benchmark comparing number of file descriptors vs. performance on something like YCSB (which is a workload I would expect would be dependent on accessing many blocks)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] block manager: start using the file cache

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: block manager: start using the file cache
......................................................................

block manager: start using the file cache

This commit integrates the file cache into both the file and log block
managers. The capacity of the cache is determined at runtime by inspecting
the RLIMIT_NOFILE resource limit; because the cache doesn't manage all open
files, we use a rather conservative 50% of the limit. This can be overridden
with a command line flag, which can also be used to disable file caching
altogether.

Other changes of note:
- Unlike the FBM, the LBM creates and opens container files for read/write
  in the same operation. Since that kind of behavior isn't supported by the
  file cache, we close the files after creating them, then reopen them
  through the cache. While inelegant, I don't expect this to be problematic.
- block_manager-stress-test now uses a PeriodicOpenFdChecker to make sure
  the file cache is working correctly. Some of the test behavior was tweaked
  to increase the number of blocks, and to avoid violating the semantics of
  the file cache.
- BlockManagerTest.CloseManyBlocksTest now uses 1000 blocks, which is enough
  for it to fail with the FBM sans file cache when the process resource
  limit is 1024 open files (the default on my Ubuntu 16.04 installation).
- When constructing a block manager, we first try to increase the
  RLIMIT_NOFILE soft limit for the process to be equal to the hard limit. On
  many systems there's enough of a gap between them that this can add a lot
  of cache capacity "for free".

Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
---
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.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
13 files changed, 314 insertions(+), 120 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] block manager: start using the file cache

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.

Change subject: block manager: start using the file cache
......................................................................


block manager: start using the file cache

This commit integrates the file cache into both the file and log block
managers. The capacity of the cache is determined at runtime by inspecting
the RLIMIT_NOFILE resource limit; because the cache doesn't manage all open
files, we use a rather conservative 50% of the limit. This can be overridden
with a command line flag, which can also be used to disable file caching
altogether.

Other changes of note:
- Unlike the FBM, the LBM creates and opens container files for read/write
  in the same operation. Since that kind of behavior isn't supported by the
  file cache, we close the files after creating them, then reopen them
  through the cache. While inelegant, I don't expect this to be problematic.
- block_manager-stress-test now uses a PeriodicOpenFdChecker to make sure
  the file cache is working correctly. Some of the test behavior was tweaked
  to increase the number of blocks, and to avoid violating the semantics of
  the file cache.
- BlockManagerTest.CloseManyBlocksTest now uses 1000 blocks, which is enough
  for it to fail with the FBM sans file cache when the process resource
  limit is 1024 open files (the default on my Ubuntu 16.04 installation).
- When constructing a block manager, we first try to increase the
  RLIMIT_NOFILE soft limit for the process to be equal to the hard limit. On
  many systems there's enough of a gap between them that this can add a lot
  of cache capacity "for free".

Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Reviewed-on: http://gerrit.cloudera.org:8080/5147
Reviewed-by: Dan Burkert <da...@apache.org>
Tested-by: Kudu Jenkins
---
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.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
13 files changed, 316 insertions(+), 120 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] block manager: start using the file cache

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: block manager: start using the file cache
......................................................................


Patch Set 4:

(1 comment)

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

PS4, Line 366: OpenExistingRWFile
> I am curious to know what's the challenge in driving the new file creation 
What makes it messy is the need for the cache to use one opening mode (CREATE_NON_EXISTING) when opening the file for the very first time, then use a different one (OPEN_EXISTING) for subsequent opens. It's more per-file state for this one small use case, so it didn't seem worth it.

Since then, Dan suggested I templatize the file cache, which makes this one-off for RWFiles a little weirder still.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] block manager: start using the file cache

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: block manager: start using the file cache
......................................................................


Patch Set 4:

(1 comment)

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

Line 61:   return FLAGS_block_manager_max_open_files;
> How do you feel about comparing the flag value against the limit?  Could sa
So, WARN if the gflag value is higher than GetOpenFileLimit()? Yeah, I can do that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] block manager: start using the file cache

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: block manager: start using the file cache
......................................................................


Patch Set 2: Verified+1

TSAN failure was KUDU-1624.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] block manager: start using the file cache

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: block manager: start using the file cache
......................................................................


Patch Set 9:

(2 comments)

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

Line 64:       env->IncreaseOpenFileLimit();
I think there is a case for calling this regardless of the max_open_files configuration.


Line 74:           "Configured file descriptor limit $0 exceeds process fd limit $1",
I think this message will be more actionable as

   Configured open file limit (block_manager_max_open_files) $0 exceeds process fd limit (ulimit) $1


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] block manager: start using the file cache

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: block manager: start using the file cache
......................................................................


Patch Set 6:

(1 comment)

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

Line 61:   return FLAGS_block_manager_max_open_files;
How do you feel about comparing the flag value against the limit?  Could save some operators by catching mistakes early instead of later in production when they actually start running out of fds.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] block manager: start using the file cache

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: block manager: start using the file cache
......................................................................


Patch Set 8:

(2 comments)

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

Line 61:     static std::once_flag once;
> I think WARN at a minimum, I was suggesting FATAL, since it seems like a ti
Alright, will change to FATAL.


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

Line 43: TAG_FLAG(block_manager_max_open_files, advanced);
> Should we mark this as unstable/evolving?  I can imagine we would want to r
Sure.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] block manager: start using the file cache

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: block manager: start using the file cache
......................................................................


Patch Set 9:

(2 comments)

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

Line 64:       env->IncreaseOpenFileLimit();
> I think there is a case for calling this regardless of the max_open_files c
Yeah, now that we're checking block_manager_max_open_files in the >0 case.


Line 74:           "Configured file descriptor limit $0 exceeds process fd limit $1",
> I think this message will be more actionable as
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] block manager: start using the file cache

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Todd Lipcon, Kudu Jenkins,

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

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

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

Change subject: block manager: start using the file cache
......................................................................

block manager: start using the file cache

This commit integrates the file cache into both the file and log block
managers. The capacity of the cache is determined at runtime by inspecting
the RLIMIT_NOFILE resource limit; because the cache doesn't manage all open
files, we use a rather conservative 50% of the limit. This can be overridden
with a command line flag, which can also be used to disable file caching
altogether.

Other changes of note:
- Unlike the FBM, the LBM creates and opens container files for read/write
  in the same operation. Since that kind of behavior isn't supported by the
  file cache, we close the files after creating them, then reopen them
  through the cache. While inelegant, I don't expect this to be problematic.
- block_manager-stress-test now uses a PeriodicOpenFdChecker to make sure
  the file cache is working correctly. Some of the test behavior was tweaked
  to increase the number of blocks, and to avoid violating the semantics of
  the file cache.
- BlockManagerTest.CloseManyBlocksTest now uses 1000 blocks, which is enough
  for it to fail with the FBM sans file cache when the process resource
  limit is 1024 open files (the default on my Ubuntu 16.04 installation).
- When constructing a block manager, we first try to increase the
  RLIMIT_NOFILE soft limit for the process to be equal to the hard limit. On
  many systems there's enough of a gap between them that this can add a lot
  of cache capacity "for free".

Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
---
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.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
13 files changed, 314 insertions(+), 118 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] block manager: start using the file cache

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: block manager: start using the file cache
......................................................................

block manager: start using the file cache

This commit integrates the file cache into both the file and log block
managers. The capacity of the cache is determined at runtime by inspecting
the RLIMIT_NOFILE resource limit; because the cache doesn't manage all open
files, we use a rather conservative 50% of the limit. This can be overridden
with a command line flag.

Other changes of note:
- Unlike the FBM, the LBM creates and opens container files for read/write
  in the same operation. Since that kind of behavior isn't supported by the
  file cache, we close the files after creating them, then reopen them
  through the cache. While inelegant, I don't expect this to be problematic.
- block_manager-stress-test now uses a PeriodicOpenFdChecker to make sure
  the file cache is working correctly. Some of the test behavior was tweaked
  to increase the number of blocks.
- BlockManagerTest.CloseManyBlocksTest now uses 1000 blocks, which is enough
  for it to fail with the FBM sans file cache when the process resource
  limit is 1024 open files (the default on my Ubuntu 16.04 installation).

Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
---
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.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
13 files changed, 150 insertions(+), 87 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] block manager: start using the file cache

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: block manager: start using the file cache
......................................................................

block manager: start using the file cache

This commit integrates the file cache into both the file and log block
managers. The capacity of the cache is determined at runtime by inspecting
the RLIMIT_NOFILE resource limit; because the cache doesn't manage all open
files, we use a rather conservative 50% of the limit. This can be overridden
with a command line flag, which can also be used to disable file caching
altogether.

Other changes of note:
- Unlike the FBM, the LBM creates and opens container files for read/write
  in the same operation. Since that kind of behavior isn't supported by the
  file cache, we close the files after creating them, then reopen them
  through the cache. While inelegant, I don't expect this to be problematic.
- block_manager-stress-test now uses a PeriodicOpenFdChecker to make sure
  the file cache is working correctly. Some of the test behavior was tweaked
  to increase the number of blocks, and to avoid violating the semantics of
  the file cache.
- BlockManagerTest.CloseManyBlocksTest now uses 1000 blocks, which is enough
  for it to fail with the FBM sans file cache when the process resource
  limit is 1024 open files (the default on my Ubuntu 16.04 installation).
- When constructing a block manager, we first try to increase the
  RLIMIT_NOFILE soft limit for the process to be equal to the hard limit. On
  many systems there's enough of a gap between them that this can add a lot
  of cache capacity "for free".

Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
---
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.cc
M src/kudu/fs/log_block_manager.h
M src/kudu/rpc/rpc-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/pb_util.cc
M src/kudu/util/pb_util.h
13 files changed, 316 insertions(+), 120 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ieeefd31eca340111bc535eac1f982290e7703a88
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>