-
Notifications
You must be signed in to change notification settings - Fork 28.7k
[SPARK-5931][CORE] Use consistent naming for time properties #5236
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
Conversation
…s to handle time strings. Updated time strings in a few places to properly parse time
Test build #29325 has finished for PR 5236 at commit
|
Test build #29329 has finished for PR 5236 at commit
|
Test build #29330 has finished for PR 5236 at commit
|
Test build #29326 has finished for PR 5236 at commit
|
Test build #29328 has finished for PR 5236 at commit
|
retest this please |
Test build #29331 has finished for PR 5236 at commit
|
Fixed ordering to fix overlap of s with ms and us.
Fixed overlap of (s) with (ms) and (us) in cascading if.
case TaskLocality.PROCESS_LOCAL => "spark.locality.wait.process" | ||
case TaskLocality.NODE_LOCAL => "spark.locality.wait.node" | ||
case TaskLocality.RACK_LOCAL => "spark.locality.wait.rack" | ||
case _ => "" |
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 should be return 0L
instead to match old behavior (probably a corner case)
Test build #30176 has finished for PR 5236 at commit
|
5d (days) | ||
1y (years) | ||
|
||
If no units are provided, the default unit specified in the documentation is assumed. |
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.
[Edit] Actually, scratch my last comment. This is probably fine.
Test build #30179 has finished for PR 5236 at commit
|
I just took a quick look at the existing configs that have units. It appears that none of them are documented, fortunately. I think this patch is more or less ready for merge once it passes tests. |
Test build #30184 has finished for PR 5236 at commit
|
Looking strong. I think that's all the comments addressed now. Going once, going twice? |
@@ -48,9 +48,9 @@ Most of the configs are the same for Spark on YARN as for other deployment modes | |||
</tr> | |||
<tr> | |||
<td><code>spark.yarn.am.waitTime</code></td> | |||
<td>100000</td> | |||
<td>100s</td> |
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.
unfortunately this needs to be 100000ms
for backward compatibility, because we stated in the docs that these configs accept raw numeric values assuming the units of the default value.
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.
(either that or we leave out that line L51 in configuration.md
)
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.
Andrew - I'd prefer to leave out that line. I think it makes things simpler and encourages transitioning to the new paradigm.
Test build #30204 has finished for PR 5236 at commit
|
* Switched to using newly added getTimeAsSeconds method (see apache#5236) * Renamed minRememberDuration to minRememberDurationS to be compatible with the examples in the pull request above.
I'm late to the party, but just want to say -- this is super cool! |
Hah. Thanks. Sent with Good (www.good.com) -----Original Message----- I'm late to the party, but just want to say -- this is super cool! — The information contained in this e-mail is confidential and/or proprietary to Capital One and/or its affiliates. The information transmitted herewith is intended only for use by the individual or entity to which it is addressed. If the reader of this message is not the intended recipient, you are hereby notified that any review, retransmission, dissemination, distribution, copying or other use of, or taking of any action in reliance upon this information is strictly prohibited. If you have received this communication in error, please contact the sender and delete the material from your computer. |
I've added new utility methods to do the conversion from times specified as e.g. 120s, 240ms, 360us to convert to a consistent internal representation. I've updated usage of these constants throughout the code to be consistent.
I believe I've captured all usages of time-based properties throughout the code. I've also updated variable names in a number of places to reflect their units for clarity and updated documentation where appropriate.