[SPARK-19721][SS] Good error message for version mismatch in log files #17070

lw-lin · 2017-02-26T03:56:54Z

Problem

There are several places where we write out version identifiers in various logs for structured streaming (usually v1). However, in the places where we check for this, we throw a confusing error message.

What changes were proposed in this pull request?

This patch made two major changes:

added a parseVersion(...) method, and based on this method, fixed the following places the way they did version checking (no other place needed to do this checking):

HDFSMetadataLog
  - CompactibleFileStreamLog  ------------> fixed with this patch
    - FileStreamSourceLog  ---------------> inherited the fix of `CompactibleFileStreamLog`
    - FileStreamSinkLog  -----------------> inherited the fix of `CompactibleFileStreamLog`
  - OffsetSeqLog  ------------------------> fixed with this patch
  - anonymous subclass in KafkaSource  ---> fixed with this patch

changed the type of FileStreamSinkLog.VERSION, FileStreamSourceLog.VERSION etc. from String to Int, so that we can identify newer versions via version > 1 instead of version != "v1"
- note this didn't break any backwards compatibility -- we are still writing out "v1" and reading back "v1"

Exception message with this patch

java.lang.IllegalStateException: Failed to read log file /private/var/folders/nn/82rmvkk568sd8p3p8tb33trw0000gn/T/spark-86867b65-0069-4ef1-b0eb-d8bd258ff5b8/0. UnsupportedLogVersion: maximum supported log version is v1, but encountered v99. The log file was produced by a newer version of Spark and cannot be read by this version. Please upgrade.
	at org.apache.spark.sql.execution.streaming.HDFSMetadataLog.get(HDFSMetadataLog.scala:202)
	at org.apache.spark.sql.execution.streaming.OffsetSeqLogSuite$$anonfun$3$$anonfun$apply$mcV$sp$2.apply(OffsetSeqLogSuite.scala:78)
	at org.apache.spark.sql.execution.streaming.OffsetSeqLogSuite$$anonfun$3$$anonfun$apply$mcV$sp$2.apply(OffsetSeqLogSuite.scala:75)
	at org.apache.spark.sql.test.SQLTestUtils$class.withTempDir(SQLTestUtils.scala:133)
	at org.apache.spark.sql.execution.streaming.OffsetSeqLogSuite.withTempDir(OffsetSeqLogSuite.scala:26)
	at org.apache.spark.sql.execution.streaming.OffsetSeqLogSuite$$anonfun$3.apply$mcV$sp(OffsetSeqLogSuite.scala:75)
	at org.apache.spark.sql.execution.streaming.OffsetSeqLogSuite$$anonfun$3.apply(OffsetSeqLogSuite.scala:75)
	at org.apache.spark.sql.execution.streaming.OffsetSeqLogSuite$$anonfun$3.apply(OffsetSeqLogSuite.scala:75)
	at org.scalatest.Transformer$$anonfun$apply$1.apply$mcV$sp(Transformer.scala:22)
	at org.scalatest.OutcomeOf$class.outcomeOf(OutcomeOf.scala:85)

How was this patch tested?

unit tests

SparkQA · 2017-02-26T05:46:50Z

Test build #73476 has finished for PR 17070 at commit dcae69b.

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

srowen · 2017-02-26T08:48:04Z

This is changing a lot of stuff to barely improve an error, and the PR has problems. I don't think this is worthwhile

srowen · 2017-02-26T08:41:17Z

external/kafka-0-10-sql/src/test/scala/org/apache/spark/sql/kafka010/KafkaSourceSuite.scala

@@ -226,7 +226,15 @@ class KafkaSourceSuite extends KafkaSourceTest {
        source.getOffset.get // Read initial offset
      }

-      assert(e.getMessage.contains("Please upgrade your Spark"))
+      Seq(


I don't think it's useful to assert about the exact message. Assert that it has key substrings.

done; thanks!

srowen · 2017-02-26T08:42:12Z

external/kafka-0-10-sql/src/main/scala/org/apache/spark/sql/kafka010/KafkaSource.scala

@@ -100,7 +100,8 @@ private[kafka010] class KafkaSource(
        override def serialize(metadata: KafkaSourceOffset, out: OutputStream): Unit = {
          out.write(0) // A zero byte is written to support Spark 2.1.0 (SPARK-19517)
          val writer = new BufferedWriter(new OutputStreamWriter(out, StandardCharsets.UTF_8))
-          writer.write(VERSION)
+          writer.write("v" + VERSION)


Write one string, or write this in 3 steps if you're worried about efficiency? rather than 2

srowen · 2017-02-26T08:46:07Z

sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/HDFSMetadataLog.scala

@@ -195,6 +196,11 @@ class HDFSMetadataLog[T <: AnyRef : ClassTag](sparkSession: SparkSession, path:
      val input = fileManager.open(batchMetadataFile)
      try {
        Some(deserialize(input))
+      } catch {
+        case ise: IllegalStateException =>


Why not just let the exception go?

the low-level exception does not know about the log file's path, and I'm trying to put it into the error message to give users very explicit information

srowen · 2017-02-26T08:46:49Z

sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/HDFSMetadataLog.scala

@@ -18,6 +18,7 @@
 package org.apache.spark.sql.execution.streaming

 import java.io._
+import java.lang.IllegalStateException


You don't need to import from java.lang

ah, what a simple mistake :-)

srowen · 2017-02-26T08:47:38Z

sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/HDFSMetadataLog.scala

+            s"is v${maxSupportedVersion}, but encountered v$version. The log file was produced " +
+            s"by a newer version of Spark and cannot be read by this version. Please upgrade.")
+        }
+        else {


Put on previous line; no need to use return

srowen · 2017-02-26T08:47:45Z

sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/HDFSMetadataLog.scala

+  private[sql] def parseVersion(text: String, maxSupportedVersion: Int): Int = {
+    if (text.length > 0 && text(0) == 'v') {
+      val version =
+        try { text.substring(1, text.length).toInt }


Brace style is wrong

lw-lin · 2017-02-26T11:56:50Z

@srowen thanks for the comments! I was trying to tackle SPARK-19721, sorry the summary just said "WIP" without a JIRA number -- adding JIRA number back.

SparkQA · 2017-02-26T13:47:01Z

Test build #73487 has finished for PR 17070 at commit 18f77b0.

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

marmbrus · 2017-02-28T01:14:03Z

/cc @zsxwing

lw-lin · 2017-03-02T13:01:10Z

@zsxwing would you take a look when you've got a minute? Thanks!

lw-lin · 2017-03-07T02:44:03Z

Jenkins retest this please

SparkQA · 2017-03-07T04:39:51Z

Test build #74063 has finished for PR 17070 at commit 18f77b0.

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

lw-lin · 2017-03-09T23:52:12Z

@zsxwing would you take a look when you've got a minute? Thanks!

zsxwing

Looks pretty good. Just one nit.

zsxwing · 2017-03-16T06:44:43Z

sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/HDFSMetadataLog.scala

+        case ise: IllegalStateException =>
+          // re-throw the exception with the log file path added
+          throw new IllegalStateException(
+            s"Failed to read log file $batchMetadataFile. ${ise.getMessage}")


nit: please also add ise as the cause.

done; thanks!

SparkQA · 2017-03-16T09:18:13Z

Test build #74648 has finished for PR 17070 at commit 32c0017.

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

zsxwing · 2017-03-16T20:05:10Z

LGTM. Merging to master and ~~2.1~~. Thanks!

zsxwing · 2017-03-16T20:07:09Z

@lw-lin there are conflicts with 2.1. Could you submit a new PR for branch-2.1?

lw-lin · 2017-03-17T01:42:32Z

@zsxwing sure, please see #17327

Fix

dcae69b

srowen reviewed Feb 26, 2017

View reviewed changes

Address Sean's comments

18f77b0

lw-lin changed the title ~~[WIP][SS] Good error message for version mismatch in log files~~ [SPARK-19721][SS] Good error message for version mismatch in log files Feb 26, 2017

marmbrus mentioned this pull request Mar 9, 2017

[SPARK-19876][SS][WIP] OneTime Trigger Executor #17219

Closed

lw-lin mentioned this pull request Mar 9, 2017

[SPARK-19873][SS] Record num shuffle partitions in offset log and enforce in next batch. #17216

Closed

zsxwing reviewed Mar 16, 2017

View reviewed changes

Address comments from Ryan

32c0017

asfgit closed this in 2ea214d Mar 16, 2017

lw-lin deleted the better-msg branch March 18, 2017 05:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-19721][SS] Good error message for version mismatch in log files #17070

[SPARK-19721][SS] Good error message for version mismatch in log files #17070

lw-lin commented Feb 26, 2017 •

edited

Loading

SparkQA commented Feb 26, 2017

srowen commented Feb 26, 2017

srowen Feb 26, 2017

lw-lin Feb 26, 2017

srowen Feb 26, 2017

lw-lin Feb 26, 2017

srowen Feb 26, 2017

lw-lin Feb 26, 2017

srowen Feb 26, 2017

lw-lin Feb 26, 2017

srowen Feb 26, 2017

lw-lin Feb 26, 2017

srowen Feb 26, 2017

lw-lin Feb 26, 2017

lw-lin commented Feb 26, 2017 •

edited

Loading

SparkQA commented Feb 26, 2017

marmbrus commented Feb 28, 2017

lw-lin commented Mar 2, 2017

lw-lin commented Mar 7, 2017

SparkQA commented Mar 7, 2017

lw-lin commented Mar 9, 2017

zsxwing left a comment

zsxwing Mar 16, 2017

lw-lin Mar 16, 2017

SparkQA commented Mar 16, 2017

zsxwing commented Mar 16, 2017 •

edited

Loading

zsxwing commented Mar 16, 2017

lw-lin commented Mar 17, 2017

[SPARK-19721][SS] Good error message for version mismatch in log files #17070

[SPARK-19721][SS] Good error message for version mismatch in log files #17070

Conversation

lw-lin commented Feb 26, 2017 • edited Loading

Problem

What changes were proposed in this pull request?

Exception message with this patch

How was this patch tested?

SparkQA commented Feb 26, 2017

srowen commented Feb 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lw-lin commented Feb 26, 2017 • edited Loading

SparkQA commented Feb 26, 2017

marmbrus commented Feb 28, 2017

lw-lin commented Mar 2, 2017

lw-lin commented Mar 7, 2017

SparkQA commented Mar 7, 2017

lw-lin commented Mar 9, 2017

zsxwing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Mar 16, 2017

zsxwing commented Mar 16, 2017 • edited Loading

zsxwing commented Mar 16, 2017

lw-lin commented Mar 17, 2017

lw-lin commented Feb 26, 2017 •

edited

Loading

lw-lin commented Feb 26, 2017 •

edited

Loading

zsxwing commented Mar 16, 2017 •

edited

Loading