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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 13 May 2019 18:10:19 +0000 Gerrit-HasComments: Yes
