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

Add docs about building downstream applications #1304

Merged
merged 2 commits into from Dec 7, 2016

Conversation

@heuermh
Copy link
Member

heuermh commented Dec 6, 2016

Fixes #291

@AmplabJenkins
Copy link

AmplabJenkins commented Dec 6, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1662/
Test PASSed.

Copy link
Member

fnothaft left a comment

Looks really good! Thanks @heuermh! Just a couple of small comments. I'll ping my additions over at you this afternoon.

Extend `BDGSparkCommand` class and implement the `run(SparkContext)` method. The `MyCommandArgs`
class defined above is provided in the constructor and specifies the generic type for `BDGSparkCommand`.
The companion object defined above is declared as a field. For access to the Apache Spark
[slf4j](http://www.slf4j.org/) Logger via the `log` field, specify `with Logging`.

This comment has been minimized.

Copy link
@fnothaft

fnothaft Dec 6, 2016

Member

Small nit: not the Spark logger anymore, since they moved that to be package private. It's the BDG utils logger now.

* Extend the ADAM CLI by [adding new commands](#commands)
* Extend the ADAM CLI by [adding new commands in an external repository](#external-commands)
* Use ADAM as a [library in new applications](#library)

This comment has been minimized.

Copy link
@fnothaft

fnothaft Dec 6, 2016

Member

Nit: extra whitespace.


Then consider making a pull request to include the new command in ADAM!


This comment has been minimized.

Copy link
@fnothaft

fnothaft Dec 6, 2016

Member

Nit: extra whitespace.


## Extend the ADAM CLI by adding new commands in an external repository {#external-commands}

To extend the ADAM CLI by adding new commands in an external repository,

This comment has been minimized.

Copy link
@fnothaft

fnothaft Dec 6, 2016

Member

This is the pattern that works through dependency injection, right? Would it be useful to link to the dependency injection tool that we use, and how we use it? I'm just thinking that if someone made a subtle error here (not sure what, but a hypothetical error) that caused the command discovery to fail, this might be useful info for debugging. Perhaps we could add this in a subsection, or something like that.

This comment has been minimized.

Copy link
@heuermh

heuermh Dec 6, 2016

Author Member

It can work though dependency injection or via the ctr as demonstrated here. The Guice example is https://github.com/heuermh/adam-commands/blob/master/src/main/scala/com/github/heuermh/adam/commands/ADAMCommandsGuiceMain.scala, not so much worth mentioning in the docs.

A complete example of this pattern can be found in the
[heuermh/adam-commands](https://github.com/heuermh/adam-examples) repository.


This comment has been minimized.

Copy link
@fnothaft

fnothaft Dec 6, 2016

Member

Nit: extra whitespace.

I'm guessing the double space before sections is intentional. No big deal if you want to keep it.

```
Create an Apache Spark configuration `SparkConf` and use it to create a new `SparkContext`.
The following serialization configuration needs to be present to register ADAM classes.

This comment has been minimized.

Copy link
@fnothaft

fnothaft Dec 6, 2016

Member

I'll open a PR against this. There's another way to do the serializer registration, e.g., if you need to add your own serializer registrations. That being said, your approach is likely the common case.

@AmplabJenkins
Copy link

AmplabJenkins commented Dec 7, 2016

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/ADAM-prb/1663/
Test PASSed.

@fnothaft fnothaft merged commit b939d23 into bigdatagenomics:master Dec 7, 2016
1 check passed
1 check passed
default Merged build finished.
Details
@fnothaft
Copy link
Member

fnothaft commented Dec 7, 2016

Thanks @heuermh! I've merged.

I'll PR my additions on here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.