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.

Sign up for GitHub

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

Jump to bottom

7_THS_displayEvents #117

Merged
merged 11 commits into from
Dec 12, 2018
Merged

7_THS_displayEvents #117

merged 11 commits into from
Dec 12, 2018

Conversation

pdtran3k6
Copy link
Contributor

No description provided.

@pdtran3k6 pdtran3k6 self-assigned this Dec 3, 2018
@pdtran3k6 pdtran3k6 changed the title 7_WIP_displayEvents 7_WIP_THS_displayEvents Dec 3, 2018
@pdtran3k6 pdtran3k6 mentioned this pull request Dec 3, 2018
Merged
@pdtran3k6 pdtran3k6 changed the title 7_WIP_THS_displayEvents 7_THS_displayEvents Dec 3, 2018
@pdtran3k6 pdtran3k6 force-pushed the 7_phattran_eventReader branch 3 times, most recently from 9609772 to 53bd24e Compare December 3, 2018 20:14
@erwa
Copy link
Contributor

erwa commented Dec 4, 2018

Hey, it's difficult to see what the incremental changes in your PR are, since they include all the changes from previous (including unmerged) PRs.

Can you push all your branches to linkedin/TonY and update the base branch of each PR so that each PR only shows the incremental change?

Ref: https://blog.github.com/2016-08-15-change-the-base-branch-of-a-pull-request/

erwa
erwa reviewed Dec 4, 2018
View reviewed changes
Copy link
Contributor

@erwa erwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. I actually realized I reviewed the dispatching code rather than the displaying code. (Another reason to push your branches to linkedin/TonY and select the base branch for your pull requests so that only incremental changes are shown.)

tony-core/src/main/java/com/linkedin/tony/util/HistoryFileUtils.java Outdated Show resolved Hide resolved
tony-core/src/main/java/com/linkedin/tony/util/HistoryFileUtils.java Outdated Show resolved Hide resolved
tony-core/src/main/java/com/linkedin/tony/util/HistoryFileUtils.java Outdated Show resolved Hide resolved
tony-core/src/main/java/com/linkedin/tony/util/HistoryFileUtils.java Outdated Show resolved Hide resolved
tony-core/src/main/java/com/linkedin/tony/util/HistoryFileUtils.java Outdated Show resolved Hide resolved
tony-core/src/main/java/com/linkedin/tony/util/HistoryFileUtils.java Outdated Show resolved Hide resolved
tony-core/src/main/resources/tony-default.xml Outdated Show resolved Hide resolved
tony-core/src/main/java/com/linkedin/tony/TonyApplicationMaster.java Show resolved Hide resolved
@pdtran3k6
Copy link
Contributor Author

pdtran3k6 commented Dec 4, 2018 •
edited
Loading

Ok, I confirmed that I have permission to push branches to linkedin/tony. I will clone and work on the linkedin/tony repo for the next PR. For now, I think you can check the changes of any of my PR by picking the last commit. @erwa

erwa
erwa approved these changes Dec 5, 2018
View reviewed changes
Copy link
Contributor

@erwa erwa left a 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 minor comments.

tony-history-server/app/controllers/JobEventPageController.java Show resolved Hide resolved
tony-history-server/app/cache/CacheWrapper.java Outdated Show resolved Hide resolved
tony-history-server/app/controllers/JobEventPageController.java Show resolved Hide resolved
tony-history-server/app/utils/ParserUtils.java Outdated Show resolved Hide resolved
@pdtran3k6 pdtran3k6 force-pushed the 7_phattran_eventReader branch 5 times, most recently from 2c93bb9 to 8b2636a Compare December 10, 2018 03:07
@erwa
Copy link
Contributor

erwa commented Dec 12, 2018

This branch also has conflicts that need manual resolution.

@pdtran3k6
Organized jobs data by intermediate/finished folders.
1e825d2
@pdtran3k6
Minor modification (constants, name changes, etc.)
62fe3f2 
Used fs.rename instead of FileUtil.copy for performance reason
@pdtran3k6
Refactored code
a52f757 
Added tony-site.xml configs into tonyConf
Moved tony.history.location out of tony-default.xml
Fixed util function + skips config check in test
@pdtran3k6
Added event schema
edaef53 
Modified reader/writer and event wrapper from MapReduce
Added licenses + generating Avro java files from schema
Write events (init + finished) to jhist file
Reverted back to 30s timeout
Removed unused import
Removed unused code
Added new key + comment in tony-default.xml
Added max attempts for event writer
Reordered init event
Refactored code
Added event handler thread
@pdtran3k6
Organized jobs data by intermediate/finished folders.
e8b4298 
5_ParserUtils tests (tony-framework#106)

* Refactored code
Added tony-site.xml configs into tonyConf
Moved tony.history.location out of tony-default.xml
Fixed util function + skips config check in test
Removed redundate else statement in startTHS.sh
Replaced history file path in application.example.conf
Used TonyConfigurationKeys instead of hardcoded string
Added all config for history server in tony-default.xml + TonyConfigurationKeys
Refactored Tony config intialization method
Removed TonyClient unused Strings

* Generate intermediate date folders if necessary for each job
Added nested folder support for reading config file.
Hardcoded file path + return Collections.emptyList()
Set proper permission to nested folders.

* Reverted back to JUnit for testing since TestNG doesn't work properly with Play
Added ParserUtils tests
Updated config for BrowserTest + JobsMetadataPageControllerTest
Added config.xml for testing
Added fail reasons for tests
Added extra slash for HDFS file path (safeguard)
Removed extra slash (BUG)
@pdtran3k6
Added event schema
cb20d30 
Modified reader/writer and event wrapper from MapReduce
Added licenses + generating Avro java files from schema
Write events (init + finished) to jhist file
Reverted back to 30s timeout
Removed unused import
Removed unused code
Added new key + comment in tony-default.xml
Added max attempts for event writer
Reordered init event
Refactored code
Added event handler thread
@pdtran3k6
Added event schema
fd25a32 
Modified reader/writer and event wrapper from MapReduce
Added licenses + generating Avro java files from schema
Write events (init + finished) to jhist file
Reverted back to 30s timeout
Removed unused import
Removed unused code
Added new key + comment in tony-default.xml
Added max attempts for event writer
Reordered init event
Refactored code
@pdtran3k6
Generate intermediate date folders if necessary for each job
00d3789 
Added nested folder support for reading config file.
Hardcoded file path + return Collections.emptyList()
Set proper permission to nested folders.
@pdtran3k6
Refactored code
a488533 
Added tony-site.xml configs into tonyConf
Moved tony.history.location out of tony-default.xml
Fixed util function + skips config check in test
@pdtran3k6
Added reader code for THS
9c5ab2e 
Updated error msg
Changed data next API
Atmpt to fix Travis build
@pdtran3k6 pdtran3k6 merged commit c738a62 into tony-framework:master Dec 12, 2018
@pdtran3k6 pdtran3k6 deleted the 7_phattran_eventReader branch December 12, 2018 20:02
erwa
erwa reviewed Dec 13, 2018
View reviewed changes
tony-core/src/main/java/com/linkedin/tony/events/EventHandler.java
public void writeEvent(BlockingQueue<Event> queue, DataFileWriter<Event> writer) {
Event event = null;
try {
event = queue.poll();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be switched to .take() and we should use interrupt to stop the event handler.

Currently, we are doing a lot of spinning:

while (!isStopped) {
  writeEvent(...);
}

is going to use a lot of CPU looping over and over again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #128 for this.

erwa
erwa reviewed Dec 13, 2018
View reviewed changes
tony-core/src/test/java/com/linkedin/tony/TestEventHandler.java
private Path jobDir = new Path("./src/test/resources/jobDir");
private TonyJobMetadata metadata = new TonyJobMetadata();

private List<Event> parseEvents(FileSystem fs, Path jobDir, TonyJobMetadata metadata) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't duplicate the code in ParserUtils.parseEvents() here. You can move ParserUtils from the tony-history-server module to the tony-core module to avoid the cyclic dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erwa erwa erwa approved these changes

@edwinalu edwinalu edwinalu left review comments

@oliverhu oliverhu Awaiting requested review from oliverhu

Assignees

@pdtran3k6 pdtran3k6

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pdtran3k6 @erwa @edwinalu