Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/3525#discussion_r24401004
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -980,33 +980,77 @@ private[spark] object Utils extends Logging {
)
}
+
+ private val TB = 1L << 40
+ private val GB = 1L << 30
+ private val MB = 1L << 20
+ private val KB = 1L << 10
+
+ private val scaleCharToFactor: Map[Char, Long] = Map(
+ 'b' -> 1L,
+ 'k' -> KB,
+ 'm' -> MB,
+ 'g' -> GB,
+ 't' -> TB
+ )
+
/**
- * Convert a Java memory parameter passed to -Xmx (such as 300m or 1g)
to a number of megabytes.
- */
- def memoryStringToMb(str: String): Int = {
+ * Convert a Java memory parameter passed to -Xmx (such as "300m" or
"1g") to a number of
+ * megabytes (or other byte-scale denominations as specified by
@outputScaleChar).
+ *
+ * For @defaultInputScaleChar and @outputScaleChar, valid values are:
'b' (bytes), 'k'
+ * (kilobytes), 'm' (megabytes), 'g' (gigabytes), and 't' (terabytes).
+ *
+ * @param str String to parse an amount of memory out of
+ * @param defaultInputScaleChar if no "scale" is provided on the end of
@str (i.e. @str is a
+ * plain numeric value), assume this scale
(default: 'b' for
+ * 'bytes')
+ * @param outputScaleChar express the output in this scale, i.e. number
of bytes, kilobytes,
+ * megabytes, or gigabytes.
+ */
+ def parseMemoryString(
+ str: String,
+ defaultInputScaleChar: Char = 'b',
+ outputScaleChar: Char = 'm'): Long = {
+
val lower = str.toLowerCase
- if (lower.endsWith("k")) {
- (lower.substring(0, lower.length-1).toLong / 1024).toInt
- } else if (lower.endsWith("m")) {
- lower.substring(0, lower.length-1).toInt
- } else if (lower.endsWith("g")) {
- lower.substring(0, lower.length-1).toInt * 1024
- } else if (lower.endsWith("t")) {
- lower.substring(0, lower.length-1).toInt * 1024 * 1024
- } else {// no suffix, so it's just a number in bytes
- (lower.toLong / 1024 / 1024).toInt
- }
+ val lastChar = lower(lower.length - 1)
+ val (num, inputScaleChar) =
+ if (lastChar.isDigit) {
+ (lower.toLong, defaultInputScaleChar)
+ } else {
+ (lower.substring(0, lower.length - 1).toLong, lastChar)
+ }
+
+ (for {
--- End diff --
Why is a for construction used here? just to handle the invalid in / out
scale param? My personal taste would be to just check that the Option[Long]
exists directly and do away with it. I can't tell how much that's just me
versus how the kids talk in Scala these days. A looping construct surprised me
as there is no loop.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
