Skip to content

[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.

Sign up for GitHub

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

Closed
ilganeli wants to merge 60 commits into from
Closed

[SPARK-5931][CORE] Use consistent naming for time properties #5236

Changes from 1 commit
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
59bf9e1
[SPARK-5931] Updated Utils and JavaUtils classes to add helper method…
Mar 27, 2015
404f8c3
Updated usage of spark.core.connection.ack.wait.timeout
Mar 27, 2015
7db6d2a
Updated usage of spark.akka.timeout
Mar 27, 2015
4933fda
Updated usage of spark.storage.blockManagerSlaveTimeout
Mar 27, 2015
c9f5cad
Updated spark.shuffle.io.retryWait
Mar 27, 2015
21ef3dd
updated spark.shuffle.sasl.timeout
Mar 27, 2015
064ebd6
Updated usage of spark.cleaner.ttl
Mar 27, 2015
7320c87
updated spark.akka.heartbeat.interval
Mar 27, 2015
272c215
Updated spark.locality.wait
Mar 27, 2015
3352d34
Updated spark.scheduler.maxRegisteredResourcesWaitingTime
Mar 27, 2015
3f1cfc8
Updated spark.scheduler.revive.interval
Mar 27, 2015
6d1518e
Upated spark.speculation.interval
Mar 27, 2015
2fcc91c
Updated spark.dynamicAllocation.executorIdleTimeout
Mar 27, 2015
5181597
Updated spark.dynamicAllocation.schedulerBacklogTimeout
Mar 27, 2015
c6a0095
Updated spark.core.connection.auth.wait.timeout
Mar 27, 2015
cde9bff
Updated spark.streaming.blockInterval
Mar 27, 2015
42477aa
Updated configuration doc with note on specifying time properties
Mar 27, 2015
9a29d8d
Fixed misuse of time in streaming context test
Mar 27, 2015
34f87c2
Update Utils.scala
ilganeli Mar 28, 2015
8f741e1
Update JavaUtils.java
ilganeli Mar 28, 2015
9e2547c
Reverting doc changes
Mar 30, 2015
499bdf0
Merge branch 'SPARK-5931' of github.com:ilganeli/spark into SPARK-5931
Mar 30, 2015
5232a36
[SPARK-5931] Changed default behavior of time string conversion.
Mar 30, 2015
3a12dd8
Updated host revceiver
Mar 30, 2015
68f4e93
Updated more files to clean up usage of default time strings
Mar 30, 2015
70ac213
Fixed remaining usages to be consistent. Updated Java-side time conve…
Mar 30, 2015
647b5ac
Udpated time conversion to use map iterator instead of if fall through
Mar 31, 2015
1c0c07c
Updated Java code to add day, minutes, and hours
Mar 31, 2015
8613631
Whitespace
Mar 31, 2015
bac9edf
More whitespace
Mar 31, 2015
1858197
Fixed bug where all time was being converted to us instead of the app…
Mar 31, 2015
3b126e1
Fixed conversion to US from seconds
Mar 31, 2015
39164f9
[SPARK-5931] Updated Java conversion to be similar to scala conversio…
Mar 31, 2015
b2fc965
replaced get or default since it's not present in this version of java
Mar 31, 2015
dd0a680
Updated scala code to call into java
Mar 31, 2015
bf779b0
Special handling of overlapping usffixes for java
Apr 1, 2015
76cfa27
[SPARK-5931] Minor nit fixes'
Apr 1, 2015
5193d5f
Resolved merge conflicts
Apr 6, 2015
6387772
Updated suffix handling to handle overlap of units more gracefully
Apr 6, 2015
19c31af
Added cleaner computation of time conversions in tests
Apr 6, 2015
ff40bfe
Updated tests to fix small bugs
Apr 6, 2015
28187bf
Convert straight to seconds
Apr 6, 2015
1465390
Nit
Apr 6, 2015
cbf41db
Got rid of thrown exceptions
Apr 7, 2015
d4efd26
Added time conversion for yarn.scheduler.heartbeat.interval-ms
Apr 8, 2015
4e48679
Fixed priority order and mixed up conversions in a couple spots
Apr 8, 2015
1a1122c
Formatting fixes and added m for use as minute formatter
Apr 8, 2015
cbd2ca6
Formatting error
Apr 8, 2015
6f651a8
Now using regexes to simplify code in parseTimeString. Introduces get…
Apr 8, 2015
7d19cdd
Added fix for possible NPE
Apr 8, 2015
dc7bd08
Fixed error in exception handling
Apr 8, 2015
69fedcc
Added test for zero
Apr 8, 2015
8927e66
Fixed handling of -1
Apr 9, 2015
642a06d
Fixed logic for invalid suffixes and addid matching test
Apr 9, 2015
25d3f52
Minor nit fixes
Apr 11, 2015
bc04e05
Minor fixes and doc updates
Apr 13, 2015
951ca2d
Made the most recent round of changes
Apr 13, 2015
f5fafcd
Doc updates
Apr 13, 2015
de3bff9
Fixing style errors
Apr 13, 2015
4526c81
Update configuration.md
Apr 13, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Added fix for possible NPE
  • Loading branch information
Ilya Ganelin committed Apr 8, 2015
commit 7d19cdd16fbe9914148331d542a2d636797a83a3
10 changes: 6 additions & 4 deletions network/common/src/main/java/org/apache/spark/network/util/JavaUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,14 +150,16 @@ private static long parseTimeString(String str, TimeUnit unit) {
Matcher m = Pattern.compile("([0-9]+)([a-z]+)?").matcher(lower);
if (m.matches()) {
val = Long.parseLong(m.group(1));
if(m.groupCount() > 1) {
if (m.group(2) != null) {
suffix = m.group(2);
}
} else {
throw new NumberFormatException("Failed to parse time string.");
else {
throw new NumberFormatException("Failed to parse time string: " + str);
}
}
Copy link
Contributor

@andrewor14 andrewor14 Apr 8, 2015

Choose a reason for hiding this comment

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

This whole method is really dense. Should we do something like the following instead so it's easier to follow:

private static long parseTimeString(String str, TimeUnit unit) {
  String lower = str.toLowerCase().trim();
  long parsedTime = -1;
  TimeUnit parsedSuffix = null;

  // If the given string does not include units, just assume the provided one
  if (lower.matches("\\d+")) {
    return Long.parseLong(lower);
  }

  // Otherwise, parse the number and the units
  for (String suffix : timeSuffixes.keySet()) {
    if (lower.endsWith(suffix)) {
      try {
        // e.g. "50ms" = 50, "3 min" = 3, "3a50s" = error
        parsedTime = Long.parseLong(str.substring(0, str.length() - suffix.length()).trim());
        if (parsedTime >= 0) {
          parsedSuffix = timeSuffixes.get(suffix);
          break;
        }
      } catch(NumberFormatException e) {
        // continue
      }
    }
  }

  // No valid time suffix found
  if (parsedSuffix == null) {
    throw new SomeException(...);
  }

  return unit.convert(parsedTime, parsedSuffix);
}

Copy link
Contributor

@andrewor14 andrewor14 Apr 8, 2015

Choose a reason for hiding this comment

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

Ugh, that was much longer than expected. It would have been a lot more concise in Scala without sacrificing any readability

Copy link
Contributor

@vanzin vanzin Apr 8, 2015

Choose a reason for hiding this comment

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

Using regexes this would be pretty short.

Matcher m = Pattern.compile("([0-9]+)([a-z]+)?");
if (m.matches()) {
   long val = Long.parseLong(m.group(1));
   String suffix = m.group(2);
}

Or something along those lines.

Copy link
Contributor

@andrewor14 andrewor14 Apr 8, 2015

Choose a reason for hiding this comment

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

yeah, do that!


return unit.convert(val, timeSuffixes.containsKey(suffix) ? timeSuffixes.get(suffix) : unit);
return unit.convert(val, (suffix != null) && timeSuffixes.containsKey(suffix) ?
Copy link
Contributor

@vanzin vanzin Apr 9, 2015

Choose a reason for hiding this comment

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

This will use the unit parameter for any suffix that is not in timeSuffixes. The containsKey check should be separate and an exception should be thrown if the suffix is not there.

The unit tests don't catch this because all the invalid suffixes have spaces in them, so the regex doesn't match.

Copy link
Author

@ilganeli ilganeli Apr 9, 2015

Choose a reason for hiding this comment

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

Marcelo - great catch! I've added a test case for this and fixed it in the latest commit.

Sent with Good (www.good.com)

-----Original Message-----
From: Marcelo Vanzin [[email protected]:[email protected]]
Sent: Wednesday, April 08, 2015 11:58 PM Eastern Standard Time
To: apache/spark
Cc: Ganelin, Ilya
Subject: Re: [spark] [SPARK-5931][CORE] Use consistent naming for time properties (#5236)

In network/common/src/main/java/org/apache/spark/network/util/JavaUtils.javahttps://github.com//pull/5236#discussion_r28033893:

  • */
  • private static long parseTimeString(String str, TimeUnit unit) {
  • String lower = str.toLowerCase().trim();
  • try {
  •  String suffix = "";

  •  long val = -1;

  •  Matcher m = Pattern.compile("(-?[0-9]+)([a-z]+)?").matcher(lower);

  •  if (m.matches()) {

  •    val = Long.parseLong(m.group(1));

  •    suffix = m.group(2);

  •  } else {

  •    throw new NumberFormatException("Failed to parse time string: " + str);

  •  }

  •  return unit.convert(val, (suffix != null) && timeSuffixes.containsKey(suffix) ?

This will use the unit parameter for any suffix that is not in timeSuffixes. The containsKey check should be separate and an exception should be thrown if the suffix is not there.

The unit tests don't catch this because all the invalid suffixes have spaces in them, so the regex doesn't match.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/5236/files#r28033893.

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.

timeSuffixes.get(suffix) : unit);
} catch (NumberFormatException e) {
String timeError = "Time must be specified as seconds (s), " +
"milliseconds (ms), microseconds (us), minutes (m or min) hour (h), or day (d). " +
Expand Down