Skip to content
This repository was archived by the owner on Aug 16, 2021. It is now read-only.

Conversation

dmius
Copy link
Collaborator

@dmius dmius commented Jun 22, 2018

No description provided.

@dmius dmius requested a review from NikolayS June 22, 2018 14:55
nancy_run.sh Outdated
then
>&2 echo "WARNING: file $AFTER_DB_INIT_CODE not found"
else
echo "$AFTER_DB_INIT_CODE found"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

это наверное только в дебаг-варианте стоит выдавать

nancy_run.sh Outdated

if [ -v AFTER_DB_INIT_CODE ]
then
if [[ ! $AFTER_DB_INIT_CODE =~ "s3://" ]]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

а else ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

разве локальный вариант (file://...) тут не основной ? зачем нам такие вещи на s3 держать?
вообще, надо ли s3?

Copy link
Collaborator Author

@dmius dmius Jun 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ну AFTER_DB_INIT_CODE код который выполняется после развертывания дампа (refresh mat view), то есть он постоянный и его имеет смысл хранить рядом с дампом имхо...

nancy_run.sh Outdated

if [ -v WORKLOAD_CUSTOM_SQL ]
then
if [[ ! $WORKLOAD_CUSTOM_SQL =~ "s3://" ]]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

тоже не хватает работы с локальным вариантом (что я вообще-то думал, будет основным!)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Всё, понял, тут наоборот проверка наличия локального файла. ОК.

Вопрос такой — стоит ли нам делать для локальных файлов префикс file:// как это делает aws cli ?

(file://filename – это filename в текущей директории или в $PATH, а file:///etc/filename с тремя слешами вначале — полный путь, в данном случае /etc/filename.)

Copy link
Collaborator Author

@dmius dmius Jun 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

С локальными файлами несовсем смысл есть, почему потому что у нас же запускается консьюмер в одном каталоге, он вызывает exec_run из другого, который в свою очередь еще и nancy_run вызывает из третьего.... то есть файл в текущей дериктории для одного для другого будет не в локальной... То есть если только для nancy_run сделать поддержку.

nancy_run.sh Outdated

if [ -v TARGET_DDL_DO ]
then
if [[ ! $TARGET_DDL_DO =~ "s3://" ]]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

логика проверки наличия файла повторяется много раз — стоит вынести в функцию


[ -v WORKLOAD_BASIS_PATH ] && ! checkPath WORKLOAD_BASIS_PATH && >&2 echo "WARNING: file $WORKLOAD_BASIS_PATH not found"

[ -v WORKLOAD_CUSTOM_SQL ] && ! checkPath WORKLOAD_CUSTOM_SQL && >&2 echo "WARNING: file $WORKLOAD_CUSTOM_SQL not found"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

тут мы считаем, что WORKLOAD_CUSTOM_SQL может быть и самим контентом, и путём к файлу, так?

а проверяется только как будто путь к файлу возможен. Значит я уже не смогу просто строку "select;" передать?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

да сейчас все через файлы

Copy link
Collaborator

@NikolayS NikolayS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

круто вышло
если переменные так и оставим указывающими на файлы, то для единообразия придётся им -path в имена надобавлять,
но непонятно всё же, насколько удобно только файловые варианты иметь,
давай попробуем с этим пожить и станет ясно

@dmius dmius merged commit 40be9ef into master Jun 25, 2018
@dmius dmius deleted the dmius-file-params branch June 25, 2018 13:17
NikolayS pushed a commit that referenced this pull request Sep 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants