-
Notifications
You must be signed in to change notification settings - Fork 594
Remove usage of NullFoo objects #1469
Remove usage of NullFoo objects #1469
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!
@@ -1,5 +1,5 @@ | |||
# uploader class for transferring the topology jar/tar files to storage | |||
heron.class.uploader: com.twitter.heron.uploader.NullUploader | |||
heron.class.uploader: com.twitter.heron.uploader.localfs.LocalFileSystemUploader |
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.
The YARN scheduler does not need to upload heron core, A use case for a no-op
uploader. Replacing it with this uploader will result in unnecessary copies of the binaries. Will it be ok to let the NullUploader
exist?
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.
I changed this because all other uploaders use the local fs uploader, which just moves it to another directory, so I thought that made sense for yarn as well. Is the yarn workflow different than all the others with respect to it's uploader needs?
$ grep -r heron.class.uploader heron/config/.
heron/config/./src/yaml/conf/aurora/uploader.yaml:heron.class.uploader: com.twitter.heron.uploader.localfs.LocalFileSystemUploader
heron/config/./src/yaml/conf/local/uploader.yaml:heron.class.uploader: com.twitter.heron.uploader.localfs.LocalFileSystemUploader
heron/config/./src/yaml/conf/localzk/uploader.yaml:heron.class.uploader: com.twitter.heron.uploader.localfs.LocalFileSystemUploader
heron/config/./src/yaml/conf/marathon/uploader.yaml:heron.class.uploader: com.twitter.heron.uploader.localfs.LocalFileSystemUploader
heron/config/./src/yaml/conf/mesos/uploader.yaml:heron.class.uploader: com.twitter.heron.uploader.localfs.LocalFileSystemUploader
heron/config/./src/yaml/conf/slurm/uploader.yaml:heron.class.uploader: com.twitter.heron.uploader.localfs.LocalFileSystemUploader
heron/config/./src/yaml/conf/yarn/uploader.yaml:heron.class.uploader: com.twitter.heron.uploader.NullUploader
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.
which means it will copy to another directory - which incurs overhead and also a configuration of directory where the file needs to be moved as well. Probably, it might be a good idea to have NullUploader.
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.
@billonahill Yes, the YARN scheduler skips the uploader step. The heron binaries are added to classpath by the launcher in this case.
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.
I've changed back to NullUploader
for now, but the proper fix is really to change the yarn impl (or the shared classes it relies on) to not invoke an uploader at all. This is a cleaner approach than calling a class written to do nothing.
* Remove usage of NullFoo objects * Remove usage of NullFoo objects
For all
Foo
interfaces that use reflection, we implement aNullFoo
object with an empty implementation so we can test classes that rely on reflection to loadFoo
s. Refactoring to use a more idiomatic approach using mock objects instead. This has the following benefits:This pr only removes usage of the
NullFoo
objects. Removing the actual classes can happen in a follow-up.