Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add FS mapper #61

Merged
merged 1 commit into from
Apr 25, 2019
Merged

Add FS mapper #61

merged 1 commit into from
Apr 25, 2019

Conversation

mik-laj
Copy link
Collaborator

@mik-laj mik-laj commented Apr 8, 2019 •
edited
Loading

Hello,

I implemented a FS mapper. Implantation does not support all functionalities (See: TODO notes) and it's not idempotent. This is the limitation of pig commands.

It's not tested on Oozie.

Reference:
https://oozie.apache.org/docs/5.1.0/WorkflowFunctionalSpec.html#a3.2.4_Fs_HDFS_action

Visualisation of converted DAG:
Screenshot 2019-04-19 at 01 01 02

Code of converted DAG:
https://gist.github.com/mik-laj/3e1f6137153825d7a4477c70ed1ee3c8

Code of workflow XML:
https://github.com/GoogleCloudPlatform/cloud-composer/blob/fs-mapper/oozie-to-airflow/examples/fs/workflow.xml

Greets

@potiuk potiuk added the enhancement New feature or request label Apr 9, 2019
@mik-laj mik-laj force-pushed the fs-mapper branch 2 times, most recently from bb4a587 to 5af8bde Compare April 15, 2019 07:16
@mik-laj mik-laj marked this pull request as ready for review April 15, 2019 07:21
@mik-laj mik-laj force-pushed the fs-mapper branch 5 times, most recently from 95acf35 to 1e26257 Compare April 15, 2019 10:40
sprzedwojski
sprzedwojski reviewed Apr 15, 2019
View reviewed changes
Copy link
Contributor

@sprzedwojski sprzedwojski left a comment

Choose a reason for hiding this comment

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

I really like it! Very good code structure, very clever tests. Well done :)
I left some minor comments.

oozie-to-airflow/examples/fs/configuration.properties Outdated Show resolved Hide resolved
oozie-to-airflow/integration/run_fs_test_with_airflow.bash Outdated Show resolved Hide resolved
oozie-to-airflow/integration/run_fs_test_with_airflow.bash Outdated Show resolved Hide resolved
oozie-to-airflow/mappers/fs_mapper.py Outdated Show resolved Hide resolved
oozie-to-airflow/mappers/fs_mapper.py Outdated Show resolved Hide resolved
oozie-to-airflow/mappers/fs_mapper.py Outdated Show resolved Hide resolved
@mik-laj mik-laj mentioned this pull request Apr 17, 2019
Merged
@mik-laj mik-laj force-pushed the fs-mapper branch 4 times, most recently from 80eb7f4 to f2b0080 Compare April 18, 2019 21:24
@mik-laj
Copy link
Collaborator Author

mik-laj commented Apr 18, 2019

@sprzedwojski I considered all the comments. I am waiting for the second round of reviews

potiuk
potiuk reviewed Apr 19, 2019
View reviewed changes
Copy link
Collaborator

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Minor comments only. Good Job @mik-laj . A lot of small improvements going in the right direction not only the FS mapper itself.

oozie-to-airflow/integration/run_fs_test_with_airflow.bash Outdated Show resolved Hide resolved
oozie-to-airflow/mappers/fs_mapper.py Outdated Show resolved Hide resolved
sprzedwojski
sprzedwojski approved these changes Apr 23, 2019
View reviewed changes
Copy link
Contributor

@sprzedwojski sprzedwojski left a comment

Choose a reason for hiding this comment

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

LGTM, two small wording issues.

oozie-to-airflow/tests/utils/test_el_utils.py Outdated Show resolved Hide resolved
oozie-to-airflow/utils/el_utils.py Outdated Show resolved Hide resolved
@mik-laj mik-laj merged commit 9ee8e93 into master Apr 25, 2019
@sprzedwojski sprzedwojski deleted the fs-mapper branch May 7, 2019 13:38
ahidalgob pushed a commit that referenced this pull request Sep 22, 2023
@ahidalgob
Add FS Mapper - close #19 (#61)
5934772 
GitOrigin-RevId: 891412831542411ed7b7d23e80ae1e3e7a60549b
Change-Id: I62a21ceea91d5dc597258a19a8cd6be911b2d0ad
ahidalgob pushed a commit that referenced this pull request Sep 22, 2023
Add FS Mapper - close #19 (#61)
5429e53 
GitOrigin-RevId: 891412831542411ed7b7d23e80ae1e3e7a60549b
Change-Id: I62a21ceea91d5dc597258a19a8cd6be911b2d0ad
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sprzedwojski sprzedwojski sprzedwojski approved these changes

@potiuk potiuk potiuk approved these changes

Assignees

@mik-laj mik-laj

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mik-laj @potiuk @sprzedwojski