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

adding the cancel method for Hive, HadoopJava, Pig #201

Merged
merged 6 commits into from
Oct 9, 2015

Conversation

johnyu0520
Copy link
Contributor

so that it kills the hadoop job that it launches

@@ -22,12 +22,15 @@
import java.io.IOException;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make this change for reportalpig and reportalhive as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reportal's pig and hive seem to use a completely different code base.
We can do it, but I don't think should be in this pull request.

In addition, I think pig and hive are mostly cleaning up after themselves already, so this PR's most material impact would be on hadoopJava processes

Copy link
Contributor

Choose a reason for hiding this comment

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

make sense. We can open a github bug to track it and handle reportal changes separately.

johnyu0520 pushed a commit to johnyu0520/azkaban-plugins that referenced this pull request Oct 8, 2015
@johnyu0520
Copy link
Contributor Author

incorporated feedback. Please check again.

Thanks!
John

@Override
public Void run() throws Exception {
HadoopJobUtils.killAllSpawnedHadoopJobs(logFilePath, log);
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit - can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

um...not having this line actually causes a compile error, hence I had to add it in in the first place.

@logiclord
Copy link
Contributor

LGTM

@davidzchen
Copy link
Contributor

LGTM. Can you rebase squash the commits before merging? Thanks!

Refactoring of common methods into HadoopSecureWrapperUtils

Allowing users to specify global job properties using reportal variables
logiclord added a commit that referenced this pull request Oct 9, 2015
adding the cancel method for Hive, HadoopJava, Pig
@logiclord logiclord merged commit e46a145 into azkaban:master Oct 9, 2015
@johnyu0520 johnyu0520 deleted the master branch October 10, 2015 00:07
@davidzchen
Copy link
Contributor

@johnyu0520 - One thing I might have failed to mention in my rebase instructions in azkaban/azkaban#517 is that in order to squash your commits, once git rebase -i (the -i flag stands for "interactive") opens your editor, you need to mark all of the commits except for the first one as fixup rather than pick. This way, git rebase will squash each commit marked as fixup with the previous commit, and as a result, you will end up with a single commit.

One other thing you can do is you can edit the commit message for your squashed commit. To do that, just use reword rather than pick for the first commit but still mark all following commits as fixup. Then, once the rebase is done, git will open your editor one more time to allow you to edit your commit message.

@johnyu0520
Copy link
Contributor Author

@davidzchen
Thanks for the note.
I basically spent the whole of Friday doing rebasing and cloning and git diff apply :)

My observation is that rebasing does not work well for

  1. a series of commits with accidental formatting changes in the middle
  2. that has someone else's commits in the middle
    That was the issue with my azkaban's multiexecutor_canary pull request. I got out of it with a git diff apply (another one of your tips)

For this patch, I was fortunate to get away with git rebase -i (which I do use from time to time over the yeras). In the normal case where commits don't drag out for that long, this process is indeed pretty light weight

johnyu0520 pushed a commit to johnyu0520/azkaban-plugins that referenced this pull request Oct 12, 2015
@davidzchen
Copy link
Contributor

Thanks, @johnyu0520! I really appreciate you taking the time to for doing the rebase. Sorry for the hassle.

The reason why I am suggesting that we rebase and squash commits for pull requests is that this model is closer to the development model followed by Apache projects (i.e. one single patch per issue), and I think it would be great to eventually get Azkaban into Apache. :)

Rebasing would definitely result in a lot of merge conflicts if your branch has greatly diverged from master. I found that it becomes much easier to do development in a feature branch (as discussed in azkaban/azkaban#517) and rebase and squash commits as often as possible, such as whenever new changes land in master. That way, there would not suddenly be a ton of merge conflicts when rebasing rebase because the feature branch would not have diverged very far from master.

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

Successfully merging this pull request may close these issues.

4 participants