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.

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

marekhorst #37 exporter module serializing actions to json #57

Conversation

marekhorst
Copy link
Member

This feature is related to new CDH5->CDH4 compatible exporter module writing actionsets as sequence files with JSON records instead of exporting them directly as protobufs to HBase.

More details can be found in #37 issue.

Please review.

…ageAction support from ActionManagerServiceFacade API since XsltInfoPackageAction provides asAtomicActions() method
…files:

* introducing SequenceFileExporterMapper and updating exporter processes to handle exporting actions encoded as jsons into sequence file
* introducing sequence file exporter workflow definition and enabling sequence file export mode in both primary and preprocessing workflows, adding distcp phase
* renaming active_export_to_hbase flag to active_export_to_actionmanager
@mkobos
Copy link
Contributor

mkobos commented Oct 18, 2015

I think that we should stick to the convention that the pull request should be named the same as the commit corresponding to this request and the commit itself should have a form of "Closes #$ISSUE_NUMBER".

@mkobos
Copy link
Contributor

mkobos commented Oct 19, 2015

I'm done with the review, please take a look.

@mkobos
Copy link
Contributor

mkobos commented Oct 19, 2015

One more thing: we should have some tests covering this functionality.

@marekhorst
Copy link
Member Author

Regarding the tests: I am all for it. This will take some time, so I will suggest creating separate issue for this task.

@marekhorst
Copy link
Member Author

I've just pushed bunch of fixes requested in comments.

@mkobos
Copy link
Contributor

mkobos commented Oct 23, 2015

Regarding the tests: I am all for it. This will take some time, so I will suggest creating separate issue for this task.

Agree. Taking into consideration that we've got a lot of close deadlines upon us, this is the only option. I think that in the future, the tests should be a standard part of pull requests.

@mkobos
Copy link
Contributor

mkobos commented Oct 23, 2015

I'm done with answering, please take a look.

@marekhorst
Copy link
Member Author

Sure. I've created #73 ticket to track test related task.

@mkobos mkobos assigned marekhorst and unassigned mkobos Oct 23, 2015
@marekhorst marekhorst assigned mkobos and unassigned marekhorst Oct 26, 2015
@marekhorst
Copy link
Member Author

I've just committed lighter version of run() method.

@mkobos mkobos assigned marekhorst and unassigned mkobos Oct 26, 2015
@mkobos
Copy link
Contributor

mkobos commented Oct 26, 2015

Still, at ~50 lines not exactly a short method, but more readable.

Looks good to me.

@marekhorst marekhorst closed this Oct 26, 2015
@marekhorst marekhorst deleted the marekhorst_#1522_exporter_module_serializing_actions_to_JSON branch October 26, 2015 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants