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 2018/03/26 20:54:39 UTC

[kudu-CR] KUDU-2377: represent resource limits as uint64 t values 

Hello Grant Henke, Todd Lipcon,

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

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

to review the following change.


Change subject: KUDU-2377: represent resource limits as uint64_t values
......................................................................

KUDU-2377: represent resource limits as uint64_t values

Turns out rlim_t is an unsigned 64-bit type, and although a root shell on my
laptop won't let me set RLIMIT_NOFILE particularly high, I can set
RLIMIT_NPROC to any value, and even to RLIM_INFINITY (-1).

So to handle every value correctly, we should really be treating these as
uint64_t rather than int64_t. That also gives us correct RLIM_INFINITY
handling for free, because -1 becomes kuint64max.

Change-Id: I7e9bb71af886f732f36a8c93876f47e7591aeadb
---
M src/kudu/fs/block_manager.cc
M src/kudu/fs/block_manager.h
M src/kudu/kserver/kserver.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/file_cache.h
M src/kudu/util/threadpool.cc
M src/kudu/util/threadpool.h
9 files changed, 42 insertions(+), 44 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7e9bb71af886f732f36a8c93876f47e7591aeadb
Gerrit-Change-Number: 9806
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] util: use uint64 t values for resource limits and adjust usages

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9806 )

Change subject: util: use uint64_t values for resource limits and adjust usages
......................................................................


Patch Set 2:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/9806/2/src/kudu/fs/block_manager.cc@94
PS2, Line 94:     if (ReadFileToString(env, "/proc/sys/fs/file-max", &buf).ok() &&
> what do you think about incorporating this into Env::GetResourceLimit so it
I'd prefer to have the Env call be lightweight and for a wrapper like the kind you're describing to sit on top, perhaps in env_util. I think if we end up with more usage of this pattern it'd make sense to refactor it into env_util, but for now it doesn't matter.


http://gerrit.cloudera.org:8080/#/c/9806/2/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

http://gerrit.cloudera.org:8080/#/c/9806/2/src/kudu/util/env_posix.cc@1590
PS2, Line 1590: 2^63 
> 2^64 right?
Whoops, was thinking of kint64max when I wrote that.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e9bb71af886f732f36a8c93876f47e7591aeadb
Gerrit-Change-Number: 9806
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 28 Mar 2018 21:43:52 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2377: represent resource limits as uint64 t values

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9806 )

Change subject: KUDU-2377: represent resource limits as uint64_t values
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/9806/1/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:

http://gerrit.cloudera.org:8080/#/c/9806/1/src/kudu/util/threadpool.h@124
PS1, Line 124:   ThreadPoolBuilder& set_min_threads(uint64_t min_threads);
> per the style guide, I think these APIs should still take signed ints, othe
Sure, but that means adjusting GetThreadPoolThreadLimit to establish kint64max as an upper bound, even if RLIMIT_NPROC is, say, kuint64max - 1. The approach I took here was to pass uint64_t all the way through to avoid such conversions, in part because we can then support not just absurdly large threadpools but EXTREMELY absurd threadpools, but more importantly because conversion code is more code.

I take it from your suggestion that you'd prefer the cost of such conversion code over the loss of signed integers here?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e9bb71af886f732f36a8c93876f47e7591aeadb
Gerrit-Change-Number: 9806
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 27 Mar 2018 22:11:16 +0000
Gerrit-HasComments: Yes

[kudu-CR] util: use uint64 t values for resource limits and adjust usages

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9806 )

Change subject: util: use uint64_t values for resource limits and adjust usages
......................................................................


Patch Set 3: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e9bb71af886f732f36a8c93876f47e7591aeadb
Gerrit-Change-Number: 9806
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 30 Mar 2018 01:29:33 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2377: represent resource limits as uint64 t values

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9806 )

Change subject: KUDU-2377: represent resource limits as uint64_t values
......................................................................


Patch Set 1:

Note: I tested this by running several unit tests in a root shell where I had set `ulimit -u unlimited` and `ulimit -u 18446744073709551614` (that's kuint64max - 1).


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e9bb71af886f732f36a8c93876f47e7591aeadb
Gerrit-Change-Number: 9806
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 26 Mar 2018 20:59:16 +0000
Gerrit-HasComments: No

[kudu-CR] util: use uint64 t values for resource limits and adjust usages

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

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

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

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

Change subject: util: use uint64_t values for resource limits and adjust usages
......................................................................

util: use uint64_t values for resource limits and adjust usages

Turns out rlim_t is an unsigned 64-bit type, and although a root shell on my
laptop won't let me set RLIMIT_NOFILE particularly high, I can set
RLIMIT_NPROC to any value, and even to RLIM_INFINITY (-1).

So to handle every value correctly, we should really be treating these as
uint64_t rather than int64_t. That also gives us correct RLIM_INFINITY
handling for free, because -1 becomes kuint64max. This also effectively
reverts commit 17aceda.

However, not all callers expect uint64_t values, so some conversion is still
necessary. I also took the opportunity to beef up how the resource limits
were used by incorporating the values of /proc/sys/kernel/pid_max,
/proc/sys/kernel/threads-max, and /proc/sys/fs/file-max as upper bounds.

Change-Id: I7e9bb71af886f732f36a8c93876f47e7591aeadb
---
M src/kudu/fs/block_manager.cc
M src/kudu/kserver/kserver.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
4 files changed, 58 insertions(+), 29 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7e9bb71af886f732f36a8c93876f47e7591aeadb
Gerrit-Change-Number: 9806
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] KUDU-2377: represent resource limits as uint64 t values

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9806 )

Change subject: KUDU-2377: represent resource limits as uint64_t values
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/9806/1/src/kudu/kserver/kserver.cc
File src/kudu/kserver/kserver.cc:

http://gerrit.cloudera.org:8080/#/c/9806/1/src/kudu/kserver/kserver.cc@95
PS1, Line 95:   uint64_t rlimit = env->GetResourceLimit(Env::ResourceLimitType::RUNNING_THREADS_PER_EUID);
Not exactly associated with this patch, but should we check /proc/sys/kernel/pid_max as well? eg on my machine pid_max is 32k even though 'ulimit -u' is 128k. In such a case we should probably respect pid_max instead.


http://gerrit.cloudera.org:8080/#/c/9806/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

http://gerrit.cloudera.org:8080/#/c/9806/1/src/kudu/util/env_posix.cc@1591
PS1, Line 1591:     return l.rlim_cur;
can we static_assert(RLIM_INFINITY == kuint64Max) or somesuch here? I think it's somewhat evil to just assume it's -1 even though I'm sure it'll never change, and it has some doc value


http://gerrit.cloudera.org:8080/#/c/9806/1/src/kudu/util/threadpool.h
File src/kudu/util/threadpool.h:

http://gerrit.cloudera.org:8080/#/c/9806/1/src/kudu/util/threadpool.h@124
PS1, Line 124:   ThreadPoolBuilder& set_min_threads(uint64_t min_threads);
per the style guide, I think these APIs should still take signed ints, otherwise if some math goes wrong and we set -1 we might end up accidentally setting a very high value. I'd rather accept signed int and be able to CHECK


http://gerrit.cloudera.org:8080/#/c/9806/1/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:

http://gerrit.cloudera.org:8080/#/c/9806/1/src/kudu/util/threadpool.cc@474
PS1, Line 474:   uint64_t capacity_remaining = max_threads_ - active_threads_ +
it also makes this calculation more error-prone with regard to overflows/underflows



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e9bb71af886f732f36a8c93876f47e7591aeadb
Gerrit-Change-Number: 9806
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 27 Mar 2018 22:00:34 +0000
Gerrit-HasComments: Yes

[kudu-CR] util: use uint64 t values for resource limits and adjust usages

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9806 )

Change subject: util: use uint64_t values for resource limits and adjust usages
......................................................................

util: use uint64_t values for resource limits and adjust usages

Turns out rlim_t is an unsigned 64-bit type, and although a root shell on my
laptop won't let me set RLIMIT_NOFILE particularly high, I can set
RLIMIT_NPROC to any value, and even to RLIM_INFINITY (-1).

So to handle every value correctly, we should really be treating these as
uint64_t rather than int64_t. That also gives us correct RLIM_INFINITY
handling for free, because -1 becomes kuint64max. This also effectively
reverts commit 17aceda.

However, not all callers expect uint64_t values, so some conversion is still
necessary. I also took the opportunity to beef up how the resource limits
were used by incorporating the values of /proc/sys/kernel/pid_max,
/proc/sys/kernel/threads-max, and /proc/sys/fs/file-max as upper bounds.

Change-Id: I7e9bb71af886f732f36a8c93876f47e7591aeadb
Reviewed-on: http://gerrit.cloudera.org:8080/9806
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/fs/block_manager.cc
M src/kudu/kserver/kserver.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
4 files changed, 58 insertions(+), 29 deletions(-)

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

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7e9bb71af886f732f36a8c93876f47e7591aeadb
Gerrit-Change-Number: 9806
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] util: use uint64 t values for resource limits and adjust usages

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9806 )

Change subject: util: use uint64_t values for resource limits and adjust usages
......................................................................


Patch Set 2:

(2 comments)

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

http://gerrit.cloudera.org:8080/#/c/9806/2/src/kudu/fs/block_manager.cc@94
PS2, Line 94:     if (ReadFileToString(env, "/proc/sys/fs/file-max", &buf).ok() &&
what do you think about incorporating this into Env::GetResourceLimit so it's not just a wrapper on getrlimit but also takes into account /proc? Logically it seems like it's part of the environment... but it's up to you if you disagree, I can also see the logic that Env should just wrap a system call.


http://gerrit.cloudera.org:8080/#/c/9806/2/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

http://gerrit.cloudera.org:8080/#/c/9806/2/src/kudu/util/env_posix.cc@1590
PS2, Line 1590: 2^63 
2^64 right?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e9bb71af886f732f36a8c93876f47e7591aeadb
Gerrit-Change-Number: 9806
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 28 Mar 2018 21:40:33 +0000
Gerrit-HasComments: Yes

[kudu-CR] util: use uint64 t values for resource limits and adjust usages

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9806 )

Change subject: util: use uint64_t values for resource limits and adjust usages
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/9806/1/src/kudu/kserver/kserver.cc
File src/kudu/kserver/kserver.cc:

http://gerrit.cloudera.org:8080/#/c/9806/1/src/kudu/kserver/kserver.cc@95
PS1, Line 95:   uint64_t rlimit = env->GetResourceLimit(Env::ResourceLimitType::RUNNING_THREADS_PER_EUID);
> Not exactly associated with this patch, but should we check /proc/sys/kerne
Done.

I also incorporated /proc/sys/fs/file-max into the block_manager.cc equivalent.


http://gerrit.cloudera.org:8080/#/c/9806/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:

http://gerrit.cloudera.org:8080/#/c/9806/1/src/kudu/util/env_posix.cc@1591
PS1, Line 1591:     return l.rlim_cur;
> can we static_assert(RLIM_INFINITY == kuint64Max) or somesuch here? I think
Done


http://gerrit.cloudera.org:8080/#/c/9806/1/src/kudu/util/threadpool.cc
File src/kudu/util/threadpool.cc:

http://gerrit.cloudera.org:8080/#/c/9806/1/src/kudu/util/threadpool.cc@474
PS1, Line 474:   uint64_t capacity_remaining = max_threads_ - active_threads_ +
> it also makes this calculation more error-prone with regard to overflows/un
Yeah, UBSAN flagged this as well.

I've reverted the threadpool changes



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7e9bb71af886f732f36a8c93876f47e7591aeadb
Gerrit-Change-Number: 9806
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 27 Mar 2018 23:50:00 +0000
Gerrit-HasComments: Yes

[kudu-CR] util: use uint64 t values for resource limits and adjust usages

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

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

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

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

Change subject: util: use uint64_t values for resource limits and adjust usages
......................................................................

util: use uint64_t values for resource limits and adjust usages

Turns out rlim_t is an unsigned 64-bit type, and although a root shell on my
laptop won't let me set RLIMIT_NOFILE particularly high, I can set
RLIMIT_NPROC to any value, and even to RLIM_INFINITY (-1).

So to handle every value correctly, we should really be treating these as
uint64_t rather than int64_t. That also gives us correct RLIM_INFINITY
handling for free, because -1 becomes kuint64max. This also effectively
reverts commit 17aceda.

However, not all callers expect uint64_t values, so some conversion is still
necessary. I also took the opportunity to beef up how the resource limits
were used by incorporating the values of /proc/sys/kernel/pid_max,
/proc/sys/kernel/threads-max, and /proc/sys/fs/file-max as upper bounds.

Change-Id: I7e9bb71af886f732f36a8c93876f47e7591aeadb
---
M src/kudu/fs/block_manager.cc
M src/kudu/kserver/kserver.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
4 files changed, 58 insertions(+), 29 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7e9bb71af886f732f36a8c93876f47e7591aeadb
Gerrit-Change-Number: 9806
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>