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 23:05:09 UTC
[kudu-CR] KUDU-2377: cap GetResourceLimit return value at kint32max
Hello Grant Henke, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/9810
to review the following change.
Change subject: KUDU-2377: cap GetResourceLimit return value at kint32max
......................................................................
KUDU-2377: cap GetResourceLimit return value at kint32max
Resource limits on UNIX are 64-bit unsigned integers but we mistreat them:
1. GetResourceLimit returns the underlying unsigned long as a int64_t.
2. The values may be further truncated into 32-bit integers. For example,
the LogBlockManager constructor truncates a int64_t value derived from
the resource limit into a 32-bit integer when constructing its FileCache.
Here's a simple workaround: let's enforce that the returned resource limit
value never exceeds kint32max. Sure, that misrepresents the underlying
system limit, but for the resources we currently define (max open files and
max threads per euid), it's impossible to actually use that much.
I manually tested this by running several unit tests in a root shell with
ulimit -u set to some high values (2^32-1 and 2^32-2).
Change-Id: I0d30ebd5447867d4227974e64786cc48fdbf2688
---
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
2 files changed, 3 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/9810/1
--
To view, visit http://gerrit.cloudera.org:8080/9810
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0d30ebd5447867d4227974e64786cc48fdbf2688
Gerrit-Change-Number: 9810
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] KUDU-2377: cap GetResourceLimit return value at kint32max
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9810 )
Change subject: KUDU-2377: cap GetResourceLimit return value at kint32max
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/9810
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d30ebd5447867d4227974e64786cc48fdbf2688
Gerrit-Change-Number: 9810
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: Tue, 27 Mar 2018 14:16:30 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2377: cap GetResourceLimit return value at kint32max
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9810 )
Change subject: KUDU-2377: cap GetResourceLimit return value at kint32max
......................................................................
KUDU-2377: cap GetResourceLimit return value at kint32max
Resource limits on UNIX are 64-bit unsigned integers but we mistreat them:
1. GetResourceLimit returns the underlying unsigned long as a int64_t.
2. The values may be further truncated into 32-bit integers. For example,
the LogBlockManager constructor truncates a int64_t value derived from
the resource limit into a 32-bit integer when constructing its FileCache.
Here's a simple workaround: let's enforce that the returned resource limit
value never exceeds kint32max. Sure, that misrepresents the underlying
system limit, but for the resources we currently define (max open files and
max threads per euid), it's impossible to actually use that much.
I manually tested this by running several unit tests in a root shell with
ulimit -u set to some high values (2^32-1 and 2^32-2).
Change-Id: I0d30ebd5447867d4227974e64786cc48fdbf2688
Reviewed-on: http://gerrit.cloudera.org:8080/9810
Tested-by: Kudu Jenkins
Reviewed-by: Grant Henke <gr...@apache.org>
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
2 files changed, 8 insertions(+), 1 deletion(-)
Approvals:
Kudu Jenkins: Verified
Grant Henke: Looks good to me, but someone else must approve
Todd Lipcon: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/9810
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I0d30ebd5447867d4227974e64786cc48fdbf2688
Gerrit-Change-Number: 9810
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>
[kudu-CR] KUDU-2377: cap GetResourceLimit return value at kint32max
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/9810
to look at the new patch set (#2).
Change subject: KUDU-2377: cap GetResourceLimit return value at kint32max
......................................................................
KUDU-2377: cap GetResourceLimit return value at kint32max
Resource limits on UNIX are 64-bit unsigned integers but we mistreat them:
1. GetResourceLimit returns the underlying unsigned long as a int64_t.
2. The values may be further truncated into 32-bit integers. For example,
the LogBlockManager constructor truncates a int64_t value derived from
the resource limit into a 32-bit integer when constructing its FileCache.
Here's a simple workaround: let's enforce that the returned resource limit
value never exceeds kint32max. Sure, that misrepresents the underlying
system limit, but for the resources we currently define (max open files and
max threads per euid), it's impossible to actually use that much.
I manually tested this by running several unit tests in a root shell with
ulimit -u set to some high values (2^32-1 and 2^32-2).
Change-Id: I0d30ebd5447867d4227974e64786cc48fdbf2688
---
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
2 files changed, 8 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/9810/2
--
To view, visit http://gerrit.cloudera.org:8080/9810
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0d30ebd5447867d4227974e64786cc48fdbf2688
Gerrit-Change-Number: 9810
Gerrit-PatchSet: 2
Gerrit-Owner: 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: cap GetResourceLimit return value at kint32max
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9810 )
Change subject: KUDU-2377: cap GetResourceLimit return value at kint32max
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9810/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/9810/1/src/kudu/util/env_posix.cc@1591
PS1, Line 1591: PCHECK(getrlimit(ResourceLimitTypeToUnixRlimit(t), &l) == 0);
> Looks like rlim_cur is a __uint64_t. Maybe it's worth a comment to specify
Like Grant said, RLIM_INFINITY is defined to be -1, which works out to 2^63-1 when converted to uint64_t and thus exceeds kint32max. This was borne out in the testing I did. Are you implying that the implicit type conversion is unsafe, and that I should add an explicit conversion somewhere here?
Anyway I added a comment too.
--
To view, visit http://gerrit.cloudera.org:8080/9810
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d30ebd5447867d4227974e64786cc48fdbf2688
Gerrit-Change-Number: 9810
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: Tue, 27 Mar 2018 04:44:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2377: cap GetResourceLimit return value at kint32max
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/9810 )
Change subject: KUDU-2377: cap GetResourceLimit return value at kint32max
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9810/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/9810/1/src/kudu/util/env_posix.cc@1591
PS1, Line 1591: return l.rlim_cur > kint32max ? kint32max : l.rlim_cur;
> Should we be checking explicitly for RLIM_INFINITY here as well? The new do
Looks like rlim_cur is a __uint64_t. Maybe it's worth a comment to specify that -1 and RLIM_INFINITY are effectively the same?
--
To view, visit http://gerrit.cloudera.org:8080/9810
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d30ebd5447867d4227974e64786cc48fdbf2688
Gerrit-Change-Number: 9810
Gerrit-PatchSet: 1
Gerrit-Owner: 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 01:17:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2377: cap GetResourceLimit return value at kint32max
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9810 )
Change subject: KUDU-2377: cap GetResourceLimit return value at kint32max
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9810
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d30ebd5447867d4227974e64786cc48fdbf2688
Gerrit-Change-Number: 9810
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: Tue, 27 Mar 2018 19:25:21 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2377: cap GetResourceLimit return value at kint32max
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9810 )
Change subject: KUDU-2377: cap GetResourceLimit return value at kint32max
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9810/1/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
http://gerrit.cloudera.org:8080/#/c/9810/1/src/kudu/util/env_posix.cc@1591
PS1, Line 1591: return l.rlim_cur > kint32max ? kint32max : l.rlim_cur;
Should we be checking explicitly for RLIM_INFINITY here as well? The new doc seems to indicate it should be converted to kint32max
--
To view, visit http://gerrit.cloudera.org:8080/9810
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0d30ebd5447867d4227974e64786cc48fdbf2688
Gerrit-Change-Number: 9810
Gerrit-PatchSet: 1
Gerrit-Owner: 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 23:55:20 +0000
Gerrit-HasComments: Yes