[SPARK-21596][SS]Ensure places calling HDFSMetadataLog.get check the return value by zsxwing · Pull Request #18799 · apache/spark

zsxwing · 2017-08-01T18:27:53Z

What changes were proposed in this pull request?

When I was investigating a flaky test, I realized that many places don't check the return value of HDFSMetadataLog.get(batchId: Long): Option[T]. When a batch is supposed to be there, the caller just ignores None rather than throwing an error. If some bug causes a query doesn't generate a batch metadata file, this behavior will hide it and allow the query continuing to run and finally delete metadata logs and make it hard to debug.

This PR ensures that places calling HDFSMetadataLog.get always check the return value.

How was this patch tested?

Jenkins

SparkQA · 2017-08-01T20:56:36Z

Test build #80129 has finished for PR 18799 at commit 7090fc0.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2017-08-02T02:45:05Z

Test build #80138 has finished for PR 18799 at commit 91efeb3.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

zsxwing · 2017-08-07T23:59:54Z

 return Some(tempPath)
 } finally {
-IOUtils.closeQuietly(output)
+output.close()


The output stream may fail to close (e.g., fail to flush the internal buffer), if it happens, we should fail the query rather than ignoring it.

tdas · 2017-08-08T00:42:32Z

- val allLogs = validBatches.flatMap(batchId => super.get(batchId)).flatten ++ logs
+ val allLogs = validBatches.map { batchId =>
+ super.get(batchId).getOrElse {
+ throw new IllegalStateException(s"batch $batchId doesn't exist")


what does it mean when this get return None? somehow the batch file is missing? is there anyway to improve the message any more?

tdas · 2017-08-08T01:40:49Z

- getAllValidBatches(latestId, compactInterval).flatMap(id => super.get(id)).flatten
+ getAllValidBatches(latestId, compactInterval).map { id =>
+ super.get(id).getOrElse {
+ throw new IllegalStateException(s"batch $id doesn't exist")


would be good give more context information. like latestId, etc.

tdas · 2017-08-08T01:41:58Z


 /**
 * Rename a path. Note that this implementation is not atomic.
+ *


is this needed?

SparkQA · 2017-08-08T02:26:58Z

Test build #80370 has finished for PR 18799 at commit 5170950.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

tdas · 2017-08-08T22:33:40Z

+ // Verify that we can get all batches between `startId` and `endId`.
+ if (startId.isDefined || endId.isDefined) {
+ if (batchIds.isEmpty) {
+ throw new IllegalStateException(s"batch ${startId.orElse(endId).get} doesn't exist")


would be good to print the range that was asked for. otherwise its hard to see what was expected while debugging.

tdas · 2017-08-08T22:34:50Z

+ test("verifyBatchIds") {
+ import HDFSMetadataLog.verifyBatchIds
+ verifyBatchIds(Seq(1L, 2L, 3L), Some(1L), Some(3L))
+ verifyBatchIds(Seq(1L), Some(1L), Some(1L))


you didnt test the valid cases when one of the start or end is None.

tdas · 2017-08-08T22:35:24Z

 val metadataLog =
 new FileStreamSourceLog(FileStreamSourceLog.VERSION, spark, dir.getAbsolutePath)
 assert(metadataLog.add(0, Array(FileEntry(s"$scheme:///file1", 100L, 0))))
+ assert(metadataLog.add(1, Array(FileEntry(s"$scheme:///file2", 200L, 0))))


what was this change for?

newSource.getBatch(None, FileStreamSourceOffset(1)) will fail without this because batch 1 doesn't exist.

SparkQA · 2017-08-08T23:49:42Z

Test build #80418 has finished for PR 18799 at commit 7f85b63.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

zsxwing · 2017-08-09T00:43:55Z

@tdas addressed your comments

tdas · 2017-08-09T01:26:34Z

LGTM.

SparkQA · 2017-08-09T03:01:38Z

Test build #80429 has finished for PR 18799 at commit 16b02da.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

tdas · 2017-08-09T03:24:40Z

Merged to master. But there were conflicts with 2.2. Can you make another PR for 2.2.

… return value When I was investigating a flaky test, I realized that many places don't check the return value of `HDFSMetadataLog.get(batchId: Long): Option[T]`. When a batch is supposed to be there, the caller just ignores None rather than throwing an error. If some bug causes a query doesn't generate a batch metadata file, this behavior will hide it and allow the query continuing to run and finally delete metadata logs and make it hard to debug. This PR ensures that places calling HDFSMetadataLog.get always check the return value. Jenkins Author: Shixiong Zhu <shixiong@databricks.com> Closesapache#18799 from zsxwing/SPARK-21596.

tdas · 2017-08-09T03:48:50Z

Never mind @zsxwing I already opened a PR #18890

… return value Same PR as #18799 but for branch 2.2. Main discussion the other PR. -------- When I was investigating a flaky test, I realized that many places don't check the return value of `HDFSMetadataLog.get(batchId: Long): Option[T]`. When a batch is supposed to be there, the caller just ignores None rather than throwing an error. If some bug causes a query doesn't generate a batch metadata file, this behavior will hide it and allow the query continuing to run and finally delete metadata logs and make it hard to debug. This PR ensures that places calling HDFSMetadataLog.get always check the return value. Jenkins Author: Shixiong Zhu <shixiong@databricks.com> Closes#18890 from tdas/SPARK-21596-2.2.

… return value Same PR as apache#18799 but for branch 2.2. Main discussion the other PR. -------- When I was investigating a flaky test, I realized that many places don't check the return value of `HDFSMetadataLog.get(batchId: Long): Option[T]`. When a batch is supposed to be there, the caller just ignores None rather than throwing an error. If some bug causes a query doesn't generate a batch metadata file, this behavior will hide it and allow the query continuing to run and finally delete metadata logs and make it hard to debug. This PR ensures that places calling HDFSMetadataLog.get always check the return value. Jenkins Author: Shixiong Zhu <shixiong@databricks.com> Closesapache#18890 from tdas/SPARK-21596-2.2.

Ensure places calling HDFSMetadataLog.get check the return value
7090fc0

One more place
91efeb3

viirya mentioned this pull request Aug 4, 2017
[SPARK-21596] Update StreamExecution.scala #18845
Closed

don't close an output stream quietly
5170950

zsxwing commented Aug 7, 2017
View reviewed changes

tdas reviewed Aug 8, 2017
View reviewed changes

Address
7f85b63

tdas reviewed Aug 8, 2017
View reviewed changes

Address more
16b02da

asfgit closed this in 6edfff0Aug 9, 2017

tdas mentioned this pull request Aug 9, 2017
[SPARK-21596][SS] Ensure places calling HDFSMetadataLog.get check the return value #18890
Closed

zsxwing deleted the SPARK-21596 branch August 9, 2017 04:04

Uh oh!

Conversation

zsxwing commented Aug 1, 2017

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Aug 1, 2017

Uh oh!

SparkQA commented Aug 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 8, 2017

Uh oh!

zsxwing commented Aug 9, 2017

Uh oh!

tdas commented Aug 9, 2017

Uh oh!

SparkQA commented Aug 9, 2017

Uh oh!

tdas commented Aug 9, 2017

Uh oh!

tdas commented Aug 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants