Abstractify hadoopy indexer configuration.#1428
Conversation
drcrallen
commented
Jun 5, 2015
drcrallen
commented
- Moves many items to JobHelper
- Remove dependencies of these functions on HadoopDruidIndexerConfig in favor of more general items
- Changes functionalities of some of the path methods to always return a path with scheme
- Adds retry to uploads
- Change output loadSpec determining from using outputFS.getClass().getName() to using outputFS.getScheme()
There was a problem hiding this comment.
I was torn on this one because if it is retrying that should perhaps just be a warning. But if something did go wrong here, I probably would want it to show up in ERROR, so I'll change it
|
It appears most of the changes are just moving methods to JobHelper and using RetryProxy versus the loops we had written before. |
There was a problem hiding this comment.
can we define these as constants?
There was a problem hiding this comment.
They were not previously, but I can
|
@fjy Yes, this is a migration rather than a major logic change. The migration is needed for upstream PRs though. |
drcrallen
force-pushed
the
abstractifyHadoopConfigStuff
branch
from
b2b79a0 to
3e0181f
Compare
|
There is one change I forgot to mention in the notes: The output loadSpec builder changed from being class.getName() based to outputFS.getScheme() based. |
* Moves many items to JobHelper * Remove dependencies of these functions on HadoopDruidIndexerConfig in favor of more general items * Changes functionalities of some of the path methods to always return a path with scheme * Adds retry to uploads * Change output loadSpec determining from using outputFS.getClass().getName() to using outputFS.getScheme()
drcrallen
force-pushed
the
abstractifyHadoopConfigStuff
branch
from
3e0181f to
2a76bdc
Compare
|
👍 |
|
LGTM, 👍 |
Abstractify hadoopy indexer configuration.
There was a problem hiding this comment.
can we explain why those "./" are necessary? They seem unnecessary, but apparently they are not?
There was a problem hiding this comment.
They are required due to Path's interpretation of "something:otherthing" as an absolute instead of relative string, so Path(somePath, "something:otherthing") will cause problems due to the later argument being an 'absolute' path instead of a relative one. But Path(somePath, "./something:otherthing") is fine
4 participants
