-
Notifications
You must be signed in to change notification settings - Fork 28
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
Added command line switch that allows saving to proto format #49
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks @lucaspcram! Nit - 2 extra lines.
*/ | ||
public class AtlasProtoOutputFormat extends AbstractFileOutputFormat<Atlas> | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: extra line
} | ||
return this.format.getRecordWriter(fileSystem, job, name, progress); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: extra line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch thanks!
I changed the switch so the generation is proto by default. Now you must provide a switch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still approving despite dismissal :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks @lucaspcram
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lucaspcram! Any way those changes could be unit-tested (or integration tested)? The other ones are not, but we need to find new ways!
@matthieun I would imagine that the best way to (integration) test this would be to kick off a local Spark job, create a single atlas file (maybe we need a test PBF file in this repo?), and then verify its format using Or are you thinking of something simpler that could fit in a unit test? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks. This is not an easy path testing this right in this PR.
Added #51 |
An optional command line switch
useJavaFormat=true
can be provided to generate atlas files using the old java serialization format. When this switch is not provided it will be set to false by default, and the new protobuf format will be used.