[SPARK-5932][CORE] Use consistent naming for size properties

[ilganeli]

I've added an interface to JavaUtils to do byte conversion and added hooks within Utils.scala to handle conversion within Spark code (like for time strings). I've added matching tests for size conversion, and then updated all deprecated configs and documentation as per SPARK-5933.

[ilganeli]

Also will need to update memoryStringToMb to use the new interface.

[SparkQA]

Test build #30520 has finished for PR 5574 at commit 09ea450.

• This patch fails to build.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[srowen]

It's pedantic, but kind of matters. I think we're all used to these memory strings from the JVM -Xmx option and refer to -Xmx4g as "4 gigabytes" but it does not give a heap of 4 x 10^9 bytes; it gives 4 x 1024^3 bytes which is really "4 gibibytes".

I think we should stick to the same convention, but, at least in docs, let's be specific that these are treated as kibibytes, mebibytes, and gibibytes, technically. I don't think we have to fix other comments or talk differently, but a bit of precision here would be useful.

[ilganeli]

Sean - I agree and in terms of the underlying math, everything is treated as kibi, gibi, etc. Which docstrings would you suggest updating? Just those for `asBytes or all the associated functions.

[srowen]

I think all of the docs you've added. It can be unobtrusive. "... converts to mebibytes (1024*1024 bytes) ..." I know this risks being confusing since most code and doc in the universe gets this technically wrong including some Spark usage, but I think it will be helpful and not annoying here.

[ilganeli]

Should we rename the enums to TiB, KiB etc? I'm personally against doing this, I think that WOULD confuse folks but it would be more consistent than it is now.

[srowen]

Tough call. I like correctness; I do think people aren't used to seeing KiB. They will probably figure it out quickly. I suppose it would help if later we needed to add support for real kilobytes somewhere, since KB isn't taken.

[SparkQA]

Test build #30523 has finished for PR 5574 at commit 851d691.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[ilganeli]

Note - Intentionally converted to 64k here instead of 0.064m since I assume this was the original intent and 0.064m does NOT equal 64k in general (mibi vs megabytes)

[SparkQA]

Test build #30530 has finished for PR 5574 at commit 0cdff35.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[vanzin]

If you want to be really correct here, you could avoid overflows by playing with the multipliers instead of converting things to bytes first.

I think what's bugging me is that the semantics of all these methods are a little weird. It seems like you're trying to cap the maximum amount to be represented to Long.MAX_VALUE bytes (so that having Long.MAX_VALUE PB, for example, would be wrong since you can't convert that to bytes). I'm not sure that's needed, but if you want that, it should be enforced differently (and not here). Otherwise, I'd rework these methods to avoid overflows where possible, and throw exceptions when they would happen.

[vanzin]

I saw your comment about using double - I don't think that's a great idea because doubles lose precision as you try to work with values at different orders of magniture.

Regarding the last paragraph of my comment above, I don't think it's going to be an issue in practice; but the code here can be changed to at least avoid overflows where possible. I checked j.u.c.TimeUnit, used in the time functions in this class, and it seems to follow the approach you took, than when an overflow is inevitable it caps the value at Long.MAX_VALUE. So that part is fine.

[ilganeli]

@vanzin Thanks for the continued feedback. I've revamped how I handle overflow and added test cases for various boundary conditions. I think this should address the prior concerns.

[SparkQA]

Test build #30878 has finished for PR 5574 at commit 852a407.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[vanzin]

As I mentioned, I do not think that using double is the correct thing. Not only the API looks weird (toBytes returns double while the others return long), but doubles do not have infinite precision.

Instead, for example, have this:

public long convertFrom(long d, ByteUnit u) {
  return u.convertTo(d, this);
}

public long convertTo(long d, ByteUnit u) {
  if (multiplier > u.multiplier) {
    long ratio = multiplier / u.multiplier;
    if (Long.MAX_VALUE / ratio < d) {
      throw new IllegalArgumentException("OVERFLOW!");
    }
    return d * ratio;
  } else {
    return d / (u.multiplier / multiplier);
  }
}

[vanzin]

Just to illustrate my concern, these are the kinds of things you run into when working with floating-point numbers:

scala> (1E15).asInstanceOf[Long] == (1E15+1).asInstanceOf[Long]
res0: Boolean = false

scala> (1E16).asInstanceOf[Long] == (1E16+1).asInstanceOf[Long]
res1: Boolean = true

[SparkQA]

Test build #30887 has finished for PR 5574 at commit fc85733.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[SparkQA]

Test build #30888 has finished for PR 5574 at commit 2ab886b.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[SparkQA]

Test build #30889 has finished for PR 5574 at commit 49a8720.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.

[ilganeli]

@andrewor14 what do you think about this in its present state? I think it should be getting near done.

[vanzin]

super nit: you could drop the JavaUtils. prefix in the call.

[vanzin]

Latest version looks good, thanks for addressing all of the feedback. Just a few nits remaining.

[SparkQA]

Test build #30946 has started for PR 5574 at commit 11f6999.

[ilganeli]

retest this please

[SparkQA]

Test build #31003 has started for PR 5574 at commit 11f6999.

[ilganeli]

@andrewor14 How does this look?

[andrewor14]

Hi @ilganeli sorry for slipping on this. I've been busy with my own patches and I will look at this right now.

[andrewor14]

LGTM I have merged this into master thanks @vanzin @ilganeli @srowen