You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2019/05/09 00:20:35 UTC
[kudu-CR] [master sentry-itest] one more scenario for authz cache
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13291
Change subject: [master_sentry-itest] one more scenario for authz cache
......................................................................
[master_sentry-itest] one more scenario for authz cache
This patch adds an extra scenario to verify the functionality of Kudu
authz system in case of HMS+Sentry integration. The newly added
scenario tries to create a table in Kudu when Sentry is down in case
of HMS+Sentry integration.
Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
---
M src/kudu/client/master_proxy_rpc.cc
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master_sentry-itest.cc
4 files changed, 64 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/13291/1
--
To view, visit http://gerrit.cloudera.org:8080/13291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
Gerrit-Change-Number: 13291
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [master sentry-itest] one more scenario for authz cache
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13291 )
Change subject: [master_sentry-itest] one more scenario for authz cache
......................................................................
Patch Set 4: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13291/3/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13291/3/src/kudu/integration-tests/master_sentry-itest.cc@908
PS3, Line 908: / CreateTable() with operation timeout longer than HMS --> Sentry
: // communication timeout successfully completes. After failing to push
: // the information on the newly created table to Sentry due to the logic
: // implemented in the SentrySyncHMSNotificationsPostEventListener plugin,
: // HMS sends success response to Kudu master and Kudu successfully completes
: // the rest of the steps.
: ASSERT_OK(CreateKuduTable(kDatabaseName, kGhostTables[0]));
:
: // In this case, the timeout for the CreateTable RPC is set to be lower than
: // the HMS --> Sentry communication timeout (see corresponding parameters
: // of the MiniHms::EnableSentry() method). CreateTable() successfully passes
: // the authz phase since the information on privileges is cached and no
: // Sentry RPC calls are attempted. However, since Sentry is down,
: // CreateTable() takes a long time on the HMS's side and the client's
: // request times out, while the creation of the table continues in the
: // background.
> Thank you for the feedback!
Yeah, returning Incomplete() Status seems find to me. We can address this in a follow up patch. Thanks a lot for working on this!
http://gerrit.cloudera.org:8080/#/c/13291/3/src/kudu/integration-tests/master_sentry-itest.cc@927
PS3, Line 927: s.IsTimedOut(
> Right, that's because the timeout for the operation was longer than the tim
ACK.
--
To view, visit http://gerrit.cloudera.org:8080/13291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
Gerrit-Change-Number: 13291
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Comment-Date: Tue, 14 May 2019 00:59:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master sentry-itest] one more scenario for authz cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13291 )
Change subject: [master_sentry-itest] one more scenario for authz cache
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
Gerrit-Change-Number: 13291
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
[kudu-CR] [master sentry-itest] one more scenario for authz cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13291 )
Change subject: [master_sentry-itest] one more scenario for authz cache
......................................................................
[master_sentry-itest] one more scenario for authz cache
This patch adds an extra scenario to verify the functionality of Kudu
authz system in case of HMS+Sentry integration. The newly added
scenario creates tables in Kudu when Sentry is temporary shut down and
the master authz cache is enabled.
Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
Reviewed-on: http://gerrit.cloudera.org:8080/13291
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Hao Hao <ha...@cloudera.com>
---
M src/kudu/client/master_proxy_rpc.cc
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master_sentry-itest.cc
6 files changed, 172 insertions(+), 17 deletions(-)
Approvals:
Alexey Serbin: Verified
Hao Hao: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/13291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
Gerrit-Change-Number: 13291
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
[kudu-CR] [master sentry-itest] one more scenario for authz cache
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13291 )
Change subject: [master_sentry-itest] one more scenario for authz cache
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
Gerrit-Change-Number: 13291
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 13 May 2019 17:28:30 +0000
Gerrit-HasComments: No
[kudu-CR] [master sentry-itest] one more scenario for authz cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13291
to look at the new patch set (#2).
Change subject: [master_sentry-itest] one more scenario for authz cache
......................................................................
[master_sentry-itest] one more scenario for authz cache
This patch adds an extra scenario to verify the functionality of Kudu
authz system in case of HMS+Sentry integration. The newly added
scenario creates tables in Kudu when Sentry is temporary shut down.
Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
---
M src/kudu/client/master_proxy_rpc.cc
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master_sentry-itest.cc
6 files changed, 166 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/13291/2
--
To view, visit http://gerrit.cloudera.org:8080/13291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
Gerrit-Change-Number: 13291
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [master sentry-itest] one more scenario for authz cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13291
to look at the new patch set (#3).
Change subject: [master_sentry-itest] one more scenario for authz cache
......................................................................
[master_sentry-itest] one more scenario for authz cache
This patch adds an extra scenario to verify the functionality of Kudu
authz system in case of HMS+Sentry integration. The newly added
scenario creates tables in Kudu when Sentry is temporary shut down.
Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
---
M src/kudu/client/master_proxy_rpc.cc
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master_sentry-itest.cc
6 files changed, 172 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/13291/3
--
To view, visit http://gerrit.cloudera.org:8080/13291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
Gerrit-Change-Number: 13291
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [master sentry-itest] one more scenario for authz cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13291
to look at the new patch set (#4).
Change subject: [master_sentry-itest] one more scenario for authz cache
......................................................................
[master_sentry-itest] one more scenario for authz cache
This patch adds an extra scenario to verify the functionality of Kudu
authz system in case of HMS+Sentry integration. The newly added
scenario creates tables in Kudu when Sentry is temporary shut down and
the master authz cache is enabled.
Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
---
M src/kudu/client/master_proxy_rpc.cc
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/integration-tests/hms_itest-base.cc
M src/kudu/integration-tests/hms_itest-base.h
M src/kudu/integration-tests/master_sentry-itest.cc
6 files changed, 172 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/13291/4
--
To view, visit http://gerrit.cloudera.org:8080/13291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
Gerrit-Change-Number: 13291
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [master sentry-itest] one more scenario for authz cache
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13291 )
Change subject: [master_sentry-itest] one more scenario for authz cache
......................................................................
Patch Set 2: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13291/2/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/13291/2/src/kudu/hms/mini_hms.h@53
PS2, Line 53: uint32_t sentry_client_rpc_retry_num = 3,
: uint32_t sentry_client_rpc_retry_interval_ms = 500);
nit: maybe add a comment explaining that these are the defaults in some version of Sentry? Or that they are set low for tests?
http://gerrit.cloudera.org:8080/#/c/13291/2/src/kudu/hms/mini_hms.h@119
PS2, Line 119: uint32_t sentry_client_rpc_retry_num_;
: uint32_t sentry_client_rpc_retry_interval_ms_;
We should try to avoid using unsigned where we can:
https://google.github.io/styleguide/cppguide.html#Integer_Types
--
To view, visit http://gerrit.cloudera.org:8080/13291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
Gerrit-Change-Number: 13291
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 10 May 2019 17:09:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master sentry-itest] one more scenario for authz cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13291 )
Change subject: [master_sentry-itest] one more scenario for authz cache
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13291/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13291/3//COMMIT_MSG@11
PS3, Line 11: when Sentry is temporary shut down
> nit: to be more specific, 'when Sentry is temporary shut down and the maste
Done
http://gerrit.cloudera.org:8080/#/c/13291/3/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13291/3/src/kudu/integration-tests/master_sentry-itest.cc@908
PS3, Line 908: / CreateTable() with operation timeout longer than HMS --> Sentry
: // communication timeout successfully completes. After failing to push
: // the information on the newly created table to Sentry due to the logic
: // implemented in the SentrySyncHMSNotificationsPostEventListener plugin,
: // HMS sends success response to Kudu master and Kudu successfully completes
: // the rest of the steps.
: ASSERT_OK(CreateKuduTable(kDatabaseName, kGhostTables[0]));
:
: // In this case, the timeout for the CreateTable RPC is set to be lower than
: // the HMS --> Sentry communication timeout (see corresponding parameters
: // of the MiniHms::EnableSentry() method). CreateTable() successfully passes
: // the authz phase since the information on privileges is cached and no
: // Sentry RPC calls are attempted. However, since Sentry is down,
: // CreateTable() takes a long time on the HMS's side and the client's
: // request times out, while the creation of the table continues in the
: // background.
> Overall I feel scenario #1 is acceptable as the table is created successful
Thank you for the feedback!
To me both scenarios are a bit strange, frankly :)
Yes, I think we can try to do something about scenario #2 to happen: e.g., we could add a special callback that removes abandoned table in case of it took longer than the client's timeout. However, that might still cause some issues with duplication of names during follow-up retries from the client that experienced timeout during table creation.
To prevent the client to automatically retry in that case, we could send response such as Incomplete() instead of Timeout() here, so the client's logic would not automatically retry and find that the table is there.
http://gerrit.cloudera.org:8080/#/c/13291/3/src/kudu/integration-tests/master_sentry-itest.cc@927
PS3, Line 927: s.IsTimedOut(
> Hmm, I remember previously it failed with 'Already present' error, as the c
Right, that's because the timeout for the operation was longer than the timeout for RPC. As I mentioned, we could address some cases with errorneous auto-retries by returning Status::Incomplete() instead of Status::TimedOut() in such cases.
--
To view, visit http://gerrit.cloudera.org:8080/13291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
Gerrit-Change-Number: 13291
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 13 May 2019 18:10:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master sentry-itest] one more scenario for authz cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13291 )
Change subject: [master_sentry-itest] one more scenario for authz cache
......................................................................
Patch Set 4: Verified+1
unrelated flakes in:
* org.apache.kudu.backup.TestOptions
* org.apache.kudu.client.TestHybridTime
* org.apache.kudu.spark.tools.TestImportExportFiles
--
To view, visit http://gerrit.cloudera.org:8080/13291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
Gerrit-Change-Number: 13291
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 13 May 2019 18:35:30 +0000
Gerrit-HasComments: No
[kudu-CR] [master sentry-itest] one more scenario for authz cache
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13291 )
Change subject: [master_sentry-itest] one more scenario for authz cache
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13291/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13291/3//COMMIT_MSG@11
PS3, Line 11: when Sentry is temporary shut down
nit: to be more specific, 'when Sentry is temporary shut down and the master authz cache is enabled'.
http://gerrit.cloudera.org:8080/#/c/13291/3/src/kudu/integration-tests/master_sentry-itest.cc
File src/kudu/integration-tests/master_sentry-itest.cc:
http://gerrit.cloudera.org:8080/#/c/13291/3/src/kudu/integration-tests/master_sentry-itest.cc@908
PS3, Line 908: / CreateTable() with operation timeout longer than HMS --> Sentry
: // communication timeout successfully completes. After failing to push
: // the information on the newly created table to Sentry due to the logic
: // implemented in the SentrySyncHMSNotificationsPostEventListener plugin,
: // HMS sends success response to Kudu master and Kudu successfully completes
: // the rest of the steps.
: ASSERT_OK(CreateKuduTable(kDatabaseName, kGhostTables[0]));
:
: // In this case, the timeout for the CreateTable RPC is set to be lower than
: // the HMS --> Sentry communication timeout (see corresponding parameters
: // of the MiniHms::EnableSentry() method). CreateTable() successfully passes
: // the authz phase since the information on privileges is cached and no
: // Sentry RPC calls are attempted. However, since Sentry is down,
: // CreateTable() takes a long time on the HMS's side and the client's
: // request times out, while the creation of the table continues in the
: // background.
Overall I feel scenario #1 is acceptable as the table is created successfully and Kudu returns Status::OK to the client, even though the owner privilege is not propagated to Sentry I think that is expected as Sentry service is down. Scenario #2 is something we might want to avoid, as the table is actually created and the client thought otherwise.
Do you have some idea how we can prevent #2 from happening? Can we pull the sentry client time out config and validate if that is lower than Kudu's client timeout?
http://gerrit.cloudera.org:8080/#/c/13291/3/src/kudu/integration-tests/master_sentry-itest.cc@927
PS3, Line 927: s.IsTimedOut(
Hmm, I remember previously it failed with 'Already present' error, as the client timeout and retried. Any reason why it is failing with 'timeout' now.
--
To view, visit http://gerrit.cloudera.org:8080/13291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
Gerrit-Change-Number: 13291
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 13 May 2019 17:40:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [master sentry-itest] one more scenario for authz cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13291 )
Change subject: [master_sentry-itest] one more scenario for authz cache
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13291/2/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/13291/2/src/kudu/hms/mini_hms.h@53
PS2, Line 53: rpc_retry_interval_ms' are used to override default settings
: // of the Sentry client used by HMS plugins. The default values for th
> nit: maybe add a comment explaining that these are the defaults in some ver
Done
http://gerrit.cloudera.org:8080/#/c/13291/2/src/kudu/hms/mini_hms.h@119
PS2, Line 119: std::string keytab_file_;
: rpc::SaslProtection::Type protection_ = rpc::S
> We should try to avoid using unsigned where we can:
Yeah, that guide, being written by Java adepts, has many strange statements. Unsigned types have a nice property to overflow safely, but signed types have no such one. De facto, size_t is unsigned, and everything related to size in stdlib is unsigned :)
Anyway, done.
--
To view, visit http://gerrit.cloudera.org:8080/13291
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3d3c04e4137afff407e4db8ee39a4495d9add3dc
Gerrit-Change-Number: 13291
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Fri, 10 May 2019 21:16:46 +0000
Gerrit-HasComments: Yes