Conversation
fenech
left a comment
fenech
left a comment
There was a problem hiding this comment.
It would also be good to add some extra tests to show the expected behaviour when application tags has zero or one elements.
shell/common/common_hive.sh
Outdated
| PARQUET_COMPRESSION "$PARQUET_COMPRESSION" \ | ||
| ORC_COMPRESSION "$ORC_COMPRESSION" \ | ||
| EXPERIMENT_ID "$EXPERIMENT_ID" | ||
| EXPERIMENT_ID "$EXPERIMENT_ID,$1" |
There was a problem hiding this comment.
This isn't the experiment ID any more - I would suggest changing the template variable to ##APPLICATION_TAGS## and then the second argument on this line would be something like "$EXPERIMENT_ID,$display_name".
shell/common/common_hive.sh
Outdated
|
|
||
| $DSH "$(get_perl_exports) | ||
| /usr/bin/perl -i -pe '$(get_hive_substitutions)' '$HIVE_SETTINGS_FILE' '$(get_hive_conf_dir)'/*" | ||
| /usr/bin/perl -i -pe '$(get_hive_substitutions $1)' '$HIVE_SETTINGS_FILE' '$(get_hive_conf_dir)'/*" |
There was a problem hiding this comment.
Missing quotes around '$1' - since this is a human-readable "display name", we should support writing it however we want.
shell/common/common_spark.sh
Outdated
| SPARK_MEMORY_OVERHEAD "$SPARK_MEMORY_OVERHEAD" \ | ||
| EXECUTOR_MEM "$EXECUTOR_MEM" \ | ||
| EXPERIMENT_ID "$EXPERIMENT_ID" | ||
| EXPERIMENT_ID "$EXPERIMENT_ID,$1" |
There was a problem hiding this comment.
As above, change the name of the template var.
shell/common/common_spark.sh
Outdated
| echo -e "$(get_local_bench_path)/spark_conf" | ||
| } | ||
|
|
||
| # $1: Benchmark name |
There was a problem hiding this comment.
I'm not completely against these types of "documentation comments" but we can also make the code clearer by assigning to a local variable display_name=$1.
shell/test/parse_yarn_output.bats
Outdated
| } | ||
| @test "sets the expected experimentId" { | ||
| run jq -r '.experimentId' <<<"${lines[1]}" | ||
| [ $output = 'test' ] |
There was a problem hiding this comment.
I would put quotes around $output, since it can be any string (we know that $status must be an integer, so not necessary there, but if you prefer to add them to be consistent then that's fine).
shell/test/parse_yarn_output.bats
Outdated
| @test "sets the expected displayName" { | ||
| run jq -r '.displayName' <<<"${lines[1]}" | ||
| [ $output = 'populatemetastore' ] | ||
| } No newline at end of file |
There was a problem hiding this comment.
Missing newline at end of file and quotes again, please.
alejandromontero
force-pushed
the
Refactor/dynamic-test-name-yarn
branch
from
1322a57 to
eae89f5
Compare
alejandromontero
force-pushed
the
Refactor/dynamic-test-name-yarn
branch
from
eae89f5 to
40b1857
Compare
2 participants
No description provided.