-
Notifications
You must be signed in to change notification settings - Fork 161
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
Tony History Server #62
Tony History Server #62
Conversation
…n3k6/TonY into phattran_tonyHistoryServer
public Result index(String jobId) { | ||
List<JobConfig> listOfConfigs = new ArrayList<>(); | ||
FileSystem myFs = getFs(); | ||
String tonyHistoryFolder = config.getString("tony.historyFolder"); |
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.
make tony.historyFolder constant
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.
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.
yup
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.
Hmm so in that case can I put Constants
in utils?
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.
Actually, on a second thought, I don't think we should put it in Constants file, as tony.historyFolder
is different for dev/prod mode.
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.
tony.historyFolder
is the name of the parameter, which is constant. The actual value (which will be different in dev/prod) comes from the application.conf
file (or later from $TONY_CONF_DIR/tony-site.xml
).
Thus, I think tony.historyFolder
can still be in the Constants
class.
tony-history-server/app/controllers/JobsConfigPageController.java
Outdated
Show resolved
Hide resolved
/** | ||
* The class handles all HDFS file operations | ||
*/ | ||
public class HdfsUtils { |
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.
this seems generic for all modules. We might need a component for all utils. Something like hadoop-common.
return localFileName; | ||
} | ||
|
||
static List<Path> getValidPaths(FileStatus[] lsJobDir, Predicate<FileStatus> fn) { |
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.
We need tests for methods in this class
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'll look into how tests are written for a Play application and I'll add some soon.
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.
These utility methods should be easy to test since you can just instantiate the input objects, directly call the methods, and then assert things about the returned output.
I think the two main things I'd like to see before we commit this are:
|
@@ -24,6 +25,12 @@ | |||
public class HdfsUtils { | |||
private static final Logger.ALogger LOG = Logger.of(HdfsUtils.class); | |||
|
|||
/** | |||
* Check to see if a HDFS path is valid. |
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.
Please define what "valid" means. It looks like this method just tests for path existence, in which case, why do we need this method? Why not just call fs.exists(filePath)
directly?
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.
You're correct, but imo for the sake of code modularity, we should have the method. Calling fs.exists
directly would require the try/catch block every time you call it, which makes the caller method more verbose than it should be.
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.
Ah, I see. Avoiding try/catch blocks is a reasonable reason for adding this method.
Can we still update the description and replace is valid
with exists
? I think that's clearer. Also, I suggest renaming the method pathExists
.
@@ -33,6 +40,12 @@ static boolean isPathValid(FileSystem fs, Path filePath) { | |||
} | |||
} | |||
|
|||
/** | |||
* Retrieve the content of the file on HDFS. |
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 think you should mention as a string
in the description.
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.
Noted. Fixed: 610a368
@@ -48,13 +61,26 @@ static String contentOfHdfsFile(FileSystem fs, Path filePath) { | |||
} | |||
} | |||
|
|||
/** | |||
* Return all paths that satisfy the <code>fn</code> predicate. |
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.
It doesn't make a difference here, but in general, {@code ...}
is preferable to using <code>...</code>
as {@code ...}
lets you embed <
and >
without escaping them as <
and >
. Ref: https://stackoverflow.com/a/1671106/1128392
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.
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.
Backticks is non-standard and won't cause the enclosed text to get rendered as monospace when Javadoc HTML is generated. Let's stick with {@code ...}
.
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.
Resolved: 63c80af
I think we don't need the method at all. We're designing the file naming conventions and folder layout, so we know what they are and can directly check for and read the expected files. (Also see my comment on HdfsUtils.java line 84.) |
There has been 59 commits in this PR.. shall we clean up the existing and move forward with new features? |
@oliverhu 💯yeah I think so too... I'll make a new branch after merging this PR. |
Yeah, I plan to review and hopefully commit this this afternoon. |
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. A few mostly minor comments that can be addressed in follow-up PRs. Merging this now.
Please do all new enhancements, feature work, and bug fixes in separate PRs (use a separate branch for each). Try to keep each PR focused on a single thing -- makes it easier to review and merge. |
Added custom script for starting THS.
Refactored + Modularized code.