use collectFirst instand of a var and iterate

[XuefengWu]

No description provided.

[AmplabJenkins]

Can one of the admins verify this patch?

[aarondav]

Will this continue to try to delete the remaining directories if one fails? That depends on the laziness of the underlying data structure, right?

This is certainly more concise, but I'm not sure it's clearer. I think the latter is more important, especially when dealing with potential failure scenarios.

[XuefengWu]

scala collection is not lazy, only scala Stream is lazy.

Yours, Xuefeng Wu 吴雪峰 敬上

On 2014年11月17日, at 上午9:21, Aaron Davidson notifications@github.com wrote:

In core/src/main/scala/org/apache/spark/util/Utils.scala:

@@ -759,18 +759,9 @@ private[spark] object Utils extends Logging {
if (file != null) {
try {
if (file.isDirectory && !isSymlink(file)) {

•      var savedIOException: IOException = null
  

•      for (child <- listFilesSafely(file)) {
  

•        try {
  

•          deleteRecursively(child)
  

•        } catch {
  

•          // In case of multiple exceptions, only last one will be thrown
  

•          case ioe: IOException => savedIOException = ioe
  

•        }
  

•      }
  

•      if (savedIOException != null) {
  

•        throw savedIOException
  

•      }
  

•      listFilesSafely(file).map(child => Try(deleteRecursively(child))).
  

  Will this continue to try to delete the remaining directories if one fails? That depends on the laziness of the underlying data structure, right?

This is certainly more concise, but I'm not sure it's clearer. I think the latter is more important, especially when dealing with potential failure scenarios.

—
Reply to this email directly or view it on GitHub.

[aarondav]

Iterators are lazy, however. If listFilesSafely were changed to return an iterator instead of list, this would subtly change behavior here. I am not for this change, as it makes the behavior less obvious. The prior form clearly indicated the intended and actual behavior; here it is not clear which one is intended, even if it is clear to the particular reader which one is actually occurring.

[XuefengWu]

But if listFilesSafely return a lazy list, the current code doest not work too. the scala for comprehension is only a sugar, it is same as map if there is one iterator.
so
for (child <- listFilesSafely(file)) { try { deleteRecursively(child) } catch { // In case of multiple exceptions, only last one will be thrown case ioe: IOException => savedIOException = ioe } }
is the same as
listFilesSafely(file).map{child => try { deleteRecursively(child) } catch { // In case of multiple exceptions, only last one will be thrown case ioe: IOException => savedIOException = ioe } }
the savedIOException always be null.

[XuefengWu]

Iterator(1,2,3).map{i => println(i);i} nothing happened for a lazy collection until consume asked.

[aarondav]

Your point about the for comprehension is incorrect in that, without a yield, it actually turns into a foreach, which expands lazy iterators:

for (i <- Iterator(1, 2,3)) { println(i) } works as expected.

[rxin]

While the old code is more verbose, I find it much more intuitive and readable so I wouldn't really change it.

[XuefengWu]

sound reasonable. thanks.

[XuefengWu]

I close this.