Laszlo Gaal has posted comments on this change. ( http://gerrit.cloudera.org:8080/21160 )
Change subject: IMPALA-12921, IMPALA-12985: Support locally built Ranger ...................................................................... Patch Set 15: (7 comments) It's a sizeable patch; I made a first pass through it. - The changes to the setup scripts (bootstrap_toolchain.py, impala-config.sh, create-test-configuration.sh) look reasonable. - I can't speak to the FE changes; I'd prefer to leave that up to folks with more expertise in that area. - I admit that the goal of including the the two .diff files (RANGER-4771.diff and IMPALA-12921_Addendum.diff) is not entirely clear to me, but this is certainly a limitation on my side. Are they present to support a specific regression test scenario (to validate that the proposed Impala fix is really effective), or are they helpers to enable all (or most) Impala tests to pass when a nondefault version of Ranger is used? I am not questioning whether the files should be there; just wishing for making it clearer in the description. In general, I think it would be useful to summarize the goals, and the non-goals of the patch. Some examples I have in mind (consider these examples, and please correct me if/where I am factually wrong): - goal: add changes to Impala source code, minicluster configuration, dataload details, etc. that allow a nondefault version of Ranger (possibly built locally) to run in the context of the minicluster, and to be used as the authZ server by Impala - non-goal: set up any automation for building Ranger from source. - non-goal: pass all Impala tests with such a nondefault version of Ranger (did I get this right?) - goal: prepare specific, isolated test scenarios to verify that the Impala fixes referenced by the Jira ticket IDs are really fixed, and the fixes don't depend on using a specific version of Ranger. http://gerrit.cloudera.org:8080/#/c/21160/15//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21160/15//COMMIT_MSG@7 PS15, Line 7: Support locally built Ranger maybe rephrase it as something like "Support running Impala with locally built Ranger" or "Support running a locally built version of Ranger in the [Impala] minicluster" ? My point is to spell out that the purpose of this patch is only to tweak the minicluster configuration and setup (and other details) to let the non-CDP Ranger version coexist with the other minicluster components, but building Ranger itself is clearly not in scope. I don't feel too strongly about this, but being more explicit might be more useful in this case than the opposite. http://gerrit.cloudera.org:8080/#/c/21160/15//COMMIT_MSG@14 PS15, Line 14: The instructions on how to build Ranger locally and point : Impala to the locally built Ranger server are provided as below. It would be worth mentioning explicitly that automating Ranger's build from source is not in the scope of this patch; this only supports using such a pre-built version if it exists. http://gerrit.cloudera.org:8080/#/c/21160/15//COMMIT_MSG@61 PS15, Line 61: tip being RANGER-4745 maybe add the Git hash as an absolute reference? http://gerrit.cloudera.org:8080/#/c/21160/15/testdata/cluster/ranger/README File testdata/cluster/ranger/README: http://gerrit.cloudera.org:8080/#/c/21160/15/testdata/cluster/ranger/README@3 PS15, Line 3: tip being IMPALA-13061 add the Git hash here as well http://gerrit.cloudera.org:8080/#/c/21160/15/testdata/cluster/ranger/README@4 PS15, Line 4: tip being RANGER-4745 add Git hash? http://gerrit.cloudera.org:8080/#/c/21160/15/testdata/cluster/ranger/README@6 PS15, Line 6: not yet supported by Apache Ranger Consider adding an XFAIL (maybe conditionally, it it's practical) for the affected test in this configuration: if it's known beforehand that the tests will fail, I don't see the point of running them. It would be great if the runtime configuration (whether the configured version of Ranger does support storage handler privileges) could be determined programmatically (so the Python test logic could just be set up to adapt automatically), but if not, a third diff-patch file could also be a solution. http://gerrit.cloudera.org:8080/#/c/21160/15/testdata/cluster/ranger/README@18 PS15, Line 18: RANGER-4745 Maybe add the Git hash for an absolute reference? Or if the applicability of the diff-patch depends on some other well-defined condition (e.g. the lack, or existence of some other patches), maybe list those, or those as well? -- To view, visit http://gerrit.cloudera.org:8080/21160 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I268d6d4d6e371da7497aac8d12f78178d57c6f27 Gerrit-Change-Number: 21160 Gerrit-PatchSet: 15 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: John Sherman <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Fri, 07 Jun 2024 19:52:36 +0000 Gerrit-HasComments: Yes
