Skip to content
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.

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

[SPARK-38723][SS][TEST][FOLLOWUP] Deflake the newly added test in QueryExecutionErrorsSuite #43565

Closed
wants to merge 3 commits into from

Conversation

WweiL
Copy link
Contributor

@WweiL WweiL commented Oct 27, 2023

What changes were proposed in this pull request?

The newly added test in 7d7afb0 could be flaky, this change deflakes it. Details see comments.

Why are the changes needed?

Deflaky

Does this PR introduce any user-facing change?

Test only change

How was this patch tested?

Test only change

Was this patch authored or co-authored using generative AI tooling?

No

@github-actions github-actions bot added the SQL label Oct 27, 2023
@WweiL
Copy link
Contributor Author

WweiL commented Oct 27, 2023

cc @HeartSaVioR

@WweiL
Copy link
Contributor Author

WweiL commented Oct 27, 2023

@HyukjinKwon Also the test should have failed in previous CI.

In the checkError method we check if parameters match when the exception has parameter

assert(expectedParameters === parameters)

So it should have thrown as:

Map("queryId" -> "1f129990-f5b0-41a9-9501-15236d130e55", "existingQueryRunId" -> "670ff74e-7ec3-4a82-a1e1-cb6d7cd58852", "newQueryRunId" -> "9571112e-91d9-43a9-b746-46fb276bbd48") did not equal Map()

The first is the parameter in the error itself, the second is empty because previously no parameter was passed in that checkError.

So it should have been checked out in CI. Could this be a build issue (like the test wasn't ran at all..?

Comment on lines 917 to 926
exceptions.map { e =>
if (e.isDefined) {
checkError(
e.get,
errorClass = "CONCURRENT_QUERY",
sqlState = Some("0A000")
sqlState = Some("0A000"),
parameters = e.get.getMessageParameters.asScala.toMap
)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you simplify this?

          exceptions.flatten.foreach { e =>
            checkError(
              e,
              errorClass = "CONCURRENT_QUERY",
              sqlState = Some("0A000"),
              parameters = e.getMessageParameters.asScala.toMap
            )
          }

@HeartSaVioR
Copy link
Contributor

Looks like the test failure in CI is irrelevant.

Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

+1

@HeartSaVioR
Copy link
Contributor

@MaxGekk Please either merge this or let me know once you're OK with the change. Thanks!

@MaxGekk
Copy link
Member

MaxGekk commented Oct 31, 2023

+1, LGTM. Merging to master.
Thank you, @WweiL and @HeartSaVioR for review.

@MaxGekk MaxGekk closed this in 97ba9d2 Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants