Lars Volker has posted comments on this change. Change subject: IMPALA-4047: DO NOT SUBMIT Highlight all occurrences of CDH/cdh ......................................................................
Patch Set 1: (35 comments) Thanks for the comments. Please see my replies. http://gerrit.cloudera.org:8080/#/c/4187/1/.gitignore File .gitignore: Line 44: xxx-*-hdfs-data/ > any idea what this is used for? remove? Doesn't seem to exist on any of my local repos. Removed. http://gerrit.cloudera.org:8080/#/c/4187/1/be/src/exec/hbase-table-scanner.cc File be/src/exec/hbase-table-scanner.cc: Line 170 > I think we can rephrase with the upstream version 0.95.2 Done http://gerrit.cloudera.org:8080/#/c/4187/1/be/src/exec/parquet-metadata-utils.h File be/src/exec/parquet-metadata-utils.h: Line 64 > Imo this information is useful because we will actually find such a string Ok, I'll keep it then. http://gerrit.cloudera.org:8080/#/c/4187/1/be/src/exec/parquet-version-test.cc File be/src/exec/parquet-version-test.cc: Line 49 > Imo leaving the cdh here is fine because these are versions strings in actu Ok http://gerrit.cloudera.org:8080/#/c/4187/1/be/src/scheduling/simple-scheduler.h File be/src/scheduling/simple-scheduler.h: Line 59 > or "Impala 3.0" We also have a Jira for this, which I will add here: https://issues.cloudera.org/browse/IMPALA-2963 http://gerrit.cloudera.org:8080/#/c/4187/1/be/src/thirdparty/squeasel/squeasel.h File be/src/thirdparty/squeasel/squeasel.h: Line 94 > Replace with https://github.com/cloudera/squeasel/blob/master/UserManual.md Done Line 185 > Replace with https://github.com/cloudera/squeasel/blob/master/UserManual.md Done http://gerrit.cloudera.org:8080/#/c/4187/1/be/src/util/debug-util.cc File be/src/util/debug-util.cc: Line 123 > Same here, it's a legacy, I think we can keep it but I'm not sure. Done http://gerrit.cloudera.org:8080/#/c/4187/1/be/src/util/redactor.detail.h File be/src/util/redactor.detail.h: Line 22 > replace with https://github.com/cloudera/logredactor Done http://gerrit.cloudera.org:8080/#/c/4187/1/be/src/util/redactor.h File be/src/util/redactor.h: Line 22 > replace with https://github.com/cloudera/logredactor Done http://gerrit.cloudera.org:8080/#/c/4187/1/bin/bootstrap_toolchain.py File bin/bootstrap_toolchain.py: > See also: https://issues.cloudera.org/browse/IMPALA-3827 , "Ensure that non Should we consider this file covered by IMPALA-3827, or should I open an own issue for it? http://gerrit.cloudera.org:8080/#/c/4187/1/bin/dump_breakpad_symbols.py File bin/dump_breakpad_symbols.py: PS1, Line 41: > Agreed. Done http://gerrit.cloudera.org:8080/#/c/4187/1/bin/start-impala-cluster.py File bin/start-impala-cluster.py: Line 220: relative_fs_cfg_path = 'xxx%s/node-%d/etc/hadoop/conf/fair-scheduler.xml' %\ > This is kind of annoying. Maybe we should rework our scripts to not even ha Just to get your point: We should remove the cdh%s prefix and have testdata/cluster/node-1 or maybe testdata/cluster/nodes/node-1 instead? If that's what you mean I'll go ahead and open a Jira for it and leave a TODO here. http://gerrit.cloudera.org:8080/#/c/4187/1/common/thrift/generate_metrics.py File common/thrift/generate_metrics.py: PS1, Line 49: parser.add_option("--output_mdl_version", dest="output_mdl_version", : metavar="IMPALA_VERSION", default="2.5.0-xxx5", : help="The Impala version that is written in the output mdl.") > 'mdl' is CM-specific Thanks. Let me know what they say and I'll create a Jira to track removal or refactoring of this. http://gerrit.cloudera.org:8080/#/c/4187/1/fe/pom.xml File fe/pom.xml: Line 198 > What is this even used for? Isn't java-cup the parser generator we use to process /fe/src/main/cup/sql-parser.cup? http://gerrit.cloudera.org:8080/#/c/4187/1/fe/src/main/java/com/cloudera/impala/analysis/SetStmt.java File fe/src/main/java/com/cloudera/impala/analysis/SetStmt.java: PS1, Line 30: > replace with Impala 2.0, which shipped with C52 Done http://gerrit.cloudera.org:8080/#/c/4187/1/fe/src/main/java/com/cloudera/impala/catalog/HBaseTable.java File fe/src/main/java/com/cloudera/impala/catalog/HBaseTable.java: PS1, Line 719: > no idea whether to keep or not This actually looks like it's long been fixed upstream (DISTRO-477 at issues.cloudera.org, HBASE-8504). I will file a Jira to work on this TODO and put the Jira ID here. http://gerrit.cloudera.org:8080/#/c/4187/1/fe/src/main/java/com/cloudera/impala/catalog/PartitionStatsUtil.java File fe/src/main/java/com/cloudera/impala/catalog/PartitionStatsUtil.java: PS1, Line 93: > Replace with Impala 2.0 Done http://gerrit.cloudera.org:8080/#/c/4187/1/fe/src/test/resources/hive-default.xml File fe/src/test/resources/hive-default.xml: PS1, Line 673: > upstream version, keep Done http://gerrit.cloudera.org:8080/#/c/4187/1/infra/deploy/deploy.py File infra/deploy/deploy.py: > My vote is to move it to a Cloudera-internal repo (Impala-aux) after ensuri Opened https://jira.cloudera.com/browse/CDH-44005 to track this. mikeb, do you want to tackle this? http://gerrit.cloudera.org:8080/#/c/4187/1/testdata/bin/create-load-data.sh File testdata/bin/create-load-data.sh: Line 266 > agree Done http://gerrit.cloudera.org:8080/#/c/4187/1/testdata/cluster/.gitignore File testdata/cluster/.gitignore: Line 1 > This feels similar like .gitignore entries for other popular tools. I'd kee Done http://gerrit.cloudera.org:8080/#/c/4187/1/testdata/cluster/admin File testdata/cluster/admin: > administering a local hdfs+yarn+llama seems general purpose to me, nut CDH Yes, I think I was wrong. There was a similar discussion in bin/start-impala-cluster.py. I will open a Jira to track getting rid of the cdh$version strings. Line 91 This could possibly go once we drop llama support. I will add a todo here mentioning IMPALA-3271. http://gerrit.cloudera.org:8080/#/c/4187/1/testdata/data/README File testdata/data/README: Line 39 > dependency version reference of sorts, I would keep it Done http://gerrit.cloudera.org:8080/#/c/4187/1/testdata/datasets/functional/functional_schema_template.sql File testdata/datasets/functional/functional_schema_template.sql: Line 1367 > dependency version of sorts, I'd keep it Done http://gerrit.cloudera.org:8080/#/c/4187/1/testdata/datasets/functional/schema_constraints.csv File testdata/datasets/functional/schema_constraints.csv: Line 126 No idea what to do here, especially I couldn't figure out what software/version we actually depend on. Ideas? http://gerrit.cloudera.org:8080/#/c/4187/1/testdata/hive_benchmark/htmlTiny/Rankings.dat File testdata/hive_benchmark/htmlTiny/Rankings.dat: > This file looks like false positives. Keep. Done http://gerrit.cloudera.org:8080/#/c/4187/1/testdata/hive_benchmark/htmlTiny/UserVisits.dat File testdata/hive_benchmark/htmlTiny/UserVisits.dat: > False positives Done http://gerrit.cloudera.org:8080/#/c/4187/1/testdata/pom.xml File testdata/pom.xml: Line 119 Looks like another dependency. Keep. http://gerrit.cloudera.org:8080/#/c/4187/1/testdata/workloads/functional-query/queries/QueryTest/parquet.test File testdata/workloads/functional-query/queries/QueryTest/parquet.test: Line 3 > dependency version of sorts, keep Done http://gerrit.cloudera.org:8080/#/c/4187/1/tests/comparison/cluster.py File tests/comparison/cluster.py: Line 166 This will go when we remove cdh$version strings here and elsewhere. Once I have a Jira for this I'll add it as a todo. PS1, Line 398: If this is a dependency of yarn, then I don't think we can change it. http://gerrit.cloudera.org:8080/#/c/4187/1/tests/comparison/leopard/impala_docker_env.py File tests/comparison/leopard/impala_docker_env.py: PS1, Line 34: : > OK then. I suggest DEFAULT_DOCKER_IMAGE_NAME be changed to 'cloudera/impala Done. I also changed DEFAULT_BRANCH_NAME to 'asf-gerrit/master'. Is origin the correct remote here or do we have a different remote name we use? Line 155 Replaced with DEFAULT_BRANCH_NAME. -- To view, visit http://gerrit.cloudera.org:8080/4187 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icb37e2ef0cd9fa0e581d359c5dd3db7812b7b2c8 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-HasComments: Yes
