Skip to content

DF/data4es: allow skipping stages#253

Merged
mgolosova merged 22 commits intomasterfrom
data4es-update
Feb 20, 2020
Merged

DF/data4es: allow skipping stages#253
mgolosova merged 22 commits intomasterfrom
data4es-update

Conversation

@mgolosova
Copy link
Copy Markdown
Collaborator

@mgolosova mgolosova commented May 31, 2019

Main changes:

  1. --ignore <stageID>[,<stageID>] option to skip specific stages.
  2. --list option to show list of stage IDs (to be used with --ignore).
  3. --help option to show usage info.
  4. Minor improvements (e.g. show usage info in case of unknown option).
  5. Lots of refactoring:
    • wrap pieces of code into functions;
    • reorder some actions;
    • get rid of hard-coded pid file name.
  6. Readability improvements (comments, etc.).

The goal is to allow data4es to utilize ES update functionality when load data.
It will allow to "disable" some ETL process nodes for faster processing and not to lose information written to ES earlier. Of course, it will make high load on the ES server, so must be used with caution; still in some situations it looks like a better solution (not to load external sources; not to request ES for known information if external source is not available or doesn`t have requested information).


ToDo:

@mgolosova mgolosova self-assigned this May 31, 2019
…ip-init' and 'origin/095-skip-init' into data4es-update
As `--ignore` accepts exact IDs, used in the code, sometimes it can be
not obvious how to specify desired ID (like in case of
'91_in'/'91_out'). New option allows to check correct spelling.
@mgolosova
Copy link
Copy Markdown
Collaborator Author

Instead of rebasing this branch on the current version of master, rewrote its history to remove "dirt" commits (appeared due to later changes in branches, that were previously merged here). Not a super-beautiful decision -- the price to pay for rebase-updates made in other branches (and for the desire to keep the merge history).

@mgolosova mgolosova changed the title [WIP] DF/data4es: allow updates DF/data4es: allow updates Sep 19, 2019
@mgolosova
Copy link
Copy Markdown
Collaborator Author

mgolosova commented Sep 19, 2019

Resolve [WIP] status for what`s left to do does not really belong here -- it is up to every individual stage.

Comment thread Utils/Dataflow/run/data4es-start Outdated
Copy link
Copy Markdown
Contributor

@Evildoor Evildoor left a comment

Choose a reason for hiding this comment

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

9f5b26b: this commit's description talks about changing "OC" to "09", but also transforms (unrelated to OC/09) line 208 into three lines (due to length exceeding 80 symbols, I presume). Shouldn't this change be a separate commit, or amendment to one of the previous ones?

0e4e8c9 and fb6c387: options --help and --list are implied, both by general logic and description, to merely output some info and exit. However, some work is actually done and, for example, checking of 095's credentials may fail and lead to script termination without info being displayed.

'09' was a "temporary name" for the Oracle Connector (OC) stage; nothing
wrong, but in the view of `--ignore` option it could be confusing. So
now '09' is the regular ID for the given stage, and 'OC' is completely
removed from the code.
snflicts:
	Utils/Dataflow/run/data4es-start
Stage commands definition was moved into a separate function to have
possibility to define all commands, get status of the operation but not
exit immediately in case of error.
…cess.

Now process status is checked only when we want to actually run the
process, not to get some information about the script usage.
This operation clearly belongs to the commands definition. Previously it
was done separately since we needed to define commands before cmdline
arguments parsing (for `--list` option), but now it's not the case.
@mgolosova
Copy link
Copy Markdown
Collaborator Author

@Evildoor,

9f5b26b: this commit's description talks about changing "OC" to "09", but also transforms (unrelated to OC/09) line 208 into three lines (due to length exceeding 80 symbols, I presume).

Thank you, fixed now.

0e4e8c9 and fb6c387: options --help and --list are implied, both by general logic and description, to merely output some info and exit. However, some work is actually done and, for example, checking of 095's credentials may fail and lead to script termination without info being displayed.

Good point, thank you; it also appeared that they failed to be properly handled in case of already running process. Fixed now.

@Evildoor
Copy link
Copy Markdown
Contributor

0e4e8c9 and fb6c387: options --help and --list are implied, both by general logic and description, to merely output some info and exit. However, some work is actually done and, for example, checking of 095's credentials may fail and lead to script termination without info being displayed.

Good point, thank you; it also appeared that they failed to be properly handled in case of already running process. Fixed now.

Branches are still created (Line 195) when these options are used, which may lead to similar problems:

[vaulov@aiatlas171 run]$ ./data4es-start --help
mkfifo: cannot create fifo Б─≤/afs/cern.ch/user/v/vaulov/Work/dkb/Utils/Dataflow/run/.data4es/.pipe/b_91Б─≥: Operation not permitted
(ERROR) 11-11-2019 15:05:05 (data4es) branch(): Failed to create named pipe for branch b_91

If script `data4es-start` is run with `--help` or `--list` parameter, it
doesn't need to create pipes for branches -- and if it does, it may fail
unexpectedly (not showing the expected usage message or list of stages).

Now this operation is moved to where all other "actual actions" are
performed (like `init_process`, etc.)
@mgolosova
Copy link
Copy Markdown
Collaborator Author

@Evildoor,

0e4e8c9 and fb6c387: options --help and --list are implied, both by general logic and description, to merely output some info and exit. However, some work is actually done and, for example, checking of 095's credentials may fail and lead to script termination without info being displayed.

Good point, thank you; it also appeared that they failed to be properly handled in case of already running process. Fixed now.

Branches are still created (Line 195) when these options are used, which may lead to similar problems:

[vaulov@aiatlas171 run]$ ./data4es-start --help
mkfifo: cannot create fifo Б─≤/afs/cern.ch/user/v/vaulov/Work/dkb/Utils/Dataflow/run/.data4es/.pipe/b_91Б─≥: Operation not permitted
(ERROR) 11-11-2019 15:05:05 (data4es) branch(): Failed to create named pipe for branch b_91

Should work properly now, please take a look.

@Evildoor
Copy link
Copy Markdown
Contributor

@Evildoor,

0e4e8c9 and fb6c387: options --help and --list are implied, both by general logic and description, to merely output some info and exit. However, some work is actually done and, for example, checking of 095's credentials may fail and lead to script termination without info being displayed.

Good point, thank you; it also appeared that they failed to be properly handled in case of already running process. Fixed now.

Branches are still created (Line 195) when these options are used, which may lead to similar problems:

[vaulov@aiatlas171 run]$ ./data4es-start --help
mkfifo: cannot create fifo Б─≤/afs/cern.ch/user/v/vaulov/Work/dkb/Utils/Dataflow/run/.data4es/.pipe/b_91Б─≥: Operation not permitted
(ERROR) 11-11-2019 15:05:05 (data4es) branch(): Failed to create named pipe for branch b_91

Should work properly now, please take a look.

Yes, works properly now, thank you.

@Evildoor
Copy link
Copy Markdown
Contributor

Evildoor commented Jan 23, 2020

Checked again: while --help works correctly, --list still does some work and produces errors:

[vaulov@aiatlas171 run]$ ./data4es-start -l
./data4es-start: line 116: source: filename argument required
source: usage: source filename [arguments]
(ERROR) 23-01-2020 13:30:22 (data4es) Stage 095: wrong authentication parameters (failed to read certificate files:  and )
09
16
17
19
25
69
91_in
91_out
93
95

Obvious difference between --help and --list is definition of stages used in the latter to get the stage numbers from variable names, which seems to be the source of the problem (aforementioned line 116).

Similar problem arises if stage 095 is added to ignore list. Also, the error message part about "certificate files: and " can be puzzling - maybe add a specific error message for the case when files are not defined?

Comment thread Utils/Dataflow/run/data4es-start Outdated
@Evildoor
Copy link
Copy Markdown
Contributor

Evildoor commented Jan 23, 2020

Comment to the PR text itself: it seems to me that four boxes starting with "make stages that might ..." should be ticked (and the last one should also link to #263) because #260, #261, #262 and #263 responsible for them were merged successfully. I'm writing this instead of fixing the text myself or ignoring it, in case that I'm wrong and something is actually not finished there.

@mgolosova
Copy link
Copy Markdown
Collaborator Author

@Evildoor,

Checked again: while --help works correctly, --list still does some work and produces errors:

[vaulov@aiatlas171 run]$ ./data4es-start -l
./data4es-start: line 116: source: filename argument required
source: usage: source filename [arguments]
(ERROR) 23-01-2020 13:30:22 (data4es) Stage 095: wrong authentication parameters (failed to > read certificate files:  and )
<...>

Similar problem arises if stage 095 is added to ignore list.
Also, the error message part about "certificate files: and " can be puzzling - maybe add a specific error message for the case when files are not defined?

I suggest to fix it in a probably unexpected way: remove handling of the files required for Stage 095 from data4es-start (check #314). This way no problem will be detected unless the stage is started -- and as a stage started, it knows what to do with these parameters in both modes ("skip" and normal processing).

Incoordination of verbs after the last change (56dfca5).
@Evildoor
Copy link
Copy Markdown
Contributor

Checked again: while --help works correctly, --list still does some work and produces errors:

[vaulov@aiatlas171 run]$ ./data4es-start -l
./data4es-start: line 116: source: filename argument required
source: usage: source filename [arguments]
(ERROR) 23-01-2020 13:30:22 (data4es) Stage 095: wrong authentication parameters (failed to > read certificate files:  and )
<...>

Similar problem arises if stage 095 is added to ignore list.
Also, the error message part about "certificate files: and " can be puzzling - maybe add a specific error message for the case when files are not defined?

I suggest to fix it in a probably unexpected way: remove handling of the files required for Stage 095 from data4es-start (check #314). This way no problem will be detected unless the stage is started -- and as a stage started, it knows what to do with these parameters in both modes ("skip" and normal processing).

Looks good to me.

Resolving conflicts:
	Utils/Dataflow/run/data4es-start
@mgolosova
Copy link
Copy Markdown
Collaborator Author

@Evildoor,

I suggest to fix it <...> (check #314).

Looks good to me.

Alright, #314 is merged now and this PR is updated.
Looks like all the issues are addressed now, please (re-)review.

Thank you!

@mgolosova mgolosova changed the title DF/data4es: allow updates DF/data4es: allow skipping stages Feb 20, 2020
@mgolosova mgolosova merged commit 4bdfd01 into master Feb 20, 2020
@mgolosova mgolosova deleted the data4es-update branch February 20, 2020 10:43
witxka added a commit that referenced this pull request Feb 26, 2020
Apply PR #253, #322, #314, #319

Conflicts:
	Utils/Dataflow/run/data4es-consistency-check
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants