Skip to content

Allow ForkingTaskRunner javaOpts to have quoted arguments which contain spaces#1748

Merged
himanshug merged 1 commit intoapache:masterfrom
metamx:forkingJavaOptionsWithQuotes
Sep 22, 2015
Merged

Allow ForkingTaskRunner javaOpts to have quoted arguments which contain spaces#1748
himanshug merged 1 commit intoapache:masterfrom
metamx:forkingJavaOptionsWithQuotes

Conversation

@drcrallen
Copy link
Copy Markdown
Contributor

It is not currently possible to pass java options to the peons for items like -XX:OnOutOfMemoryError="kill -9 %p" this PR is intended to allow such options to be passed to the peons.

@drcrallen drcrallen force-pushed the forkingJavaOptionsWithQuotes branch 2 times, most recently from c80025d to 03ece75 Compare September 17, 2015 21:52
@drcrallen drcrallen closed this Sep 17, 2015
@drcrallen drcrallen reopened this Sep 17, 2015
@drcrallen drcrallen closed this Sep 18, 2015
@drcrallen drcrallen reopened this Sep 18, 2015
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

came across http://stackoverflow.com/a/19946701 , seems simpler ... see if it makes sense to use the CharMatcher.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Tweaked with it a tiny bit, and changed some of the unit tests. Overall I think it is cleaner.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@drcrallen drcrallen force-pushed the forkingJavaOptionsWithQuotes branch 2 times, most recently from 30bc257 to 43d2d01 Compare September 18, 2015 15:50
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it is non trivial to understand why this throws AssertionError and testPatternMatcherPreservesNonBreakingSpacesJavaOptions() does not. can you pls add some more comments?
or in the other test can you put the non-breaking character used in the comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will add comments.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hopefully more clear now.

@drcrallen drcrallen force-pushed the forkingJavaOptionsWithQuotes branch from 43d2d01 to 479dadf Compare September 21, 2015 22:47
@drcrallen drcrallen added this to the 0.8.2 milestone Sep 22, 2015
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

typo

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

@drcrallen drcrallen force-pushed the forkingJavaOptionsWithQuotes branch from 479dadf to 465035e Compare September 22, 2015 00:32
@xvrl
Copy link
Copy Markdown
Member

xvrl commented Sep 22, 2015

👍

@himanshug himanshug closed this Sep 22, 2015
@himanshug himanshug reopened this Sep 22, 2015
@drcrallen
Copy link
Copy Markdown
Contributor Author

RemoteTaskRunnerTest.testWorkerRemoved:370 null

@drcrallen drcrallen closed this Sep 22, 2015
@drcrallen drcrallen reopened this Sep 22, 2015
himanshug added a commit that referenced this pull request Sep 22, 2015
Allow ForkingTaskRunner javaOpts to have quoted arguments which contain spaces
@himanshug himanshug merged commit 61b0743 into apache:master Sep 22, 2015
@drcrallen drcrallen deleted the forkingJavaOptionsWithQuotes branch September 22, 2015 02:03
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.

3 participants