You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "chaoqin-li1123 (via GitHub)" <gi...@apache.org> on 2023/11/16 05:50:10 UTC
[PR] [SPARK-45511] fix state reader suite flakiness by clean up resources after each test run [spark]
chaoqin-li1123 opened a new pull request, #43831:
URL: https://github.com/apache/spark/pull/43831
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
Fix state reader suite flakiness by clean up resources after each test. Because all state store instance share the same maintainence task pool, the failed maintainence task from previous test run may affect future test runs and cause test failure. Clean up StateStore explicitly to unflake the test.
### Why are the changes needed?
To unflake the test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45511][SS] Fix state reader suite flakiness by clean up resources after each test run [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #43831:
URL: https://github.com/apache/spark/pull/43831#discussion_r1395340212
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/state/StateDataSourceTestBase.scala:
##########
@@ -18,16 +18,27 @@ package org.apache.spark.sql.execution.datasources.v2.state
import java.sql.Timestamp
+import org.scalatest.BeforeAndAfter
+
import org.apache.spark.sql.{DataFrame, Dataset}
import org.apache.spark.sql.execution.streaming.MemoryStream
+import org.apache.spark.sql.execution.streaming.state.StateStore
import org.apache.spark.sql.functions._
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.streaming._
import org.apache.spark.sql.streaming.util.StreamManualClock
-trait StateDataSourceTestBase extends StreamTest with StateStoreMetricsTest {
+trait StateDataSourceTestBase extends StreamTest with BeforeAndAfter with StateStoreMetricsTest {
Review Comment:
I've just updated the description of PR to have full context on the problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45511][SS] Fix state reader suite flakiness by clean up resources after each test run [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #43831:
URL: https://github.com/apache/spark/pull/43831#discussion_r1395304223
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/state/StateDataSourceTestBase.scala:
##########
@@ -18,16 +18,27 @@ package org.apache.spark.sql.execution.datasources.v2.state
import java.sql.Timestamp
+import org.scalatest.BeforeAndAfter
+
import org.apache.spark.sql.{DataFrame, Dataset}
import org.apache.spark.sql.execution.streaming.MemoryStream
+import org.apache.spark.sql.execution.streaming.state.StateStore
import org.apache.spark.sql.functions._
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.streaming._
import org.apache.spark.sql.streaming.util.StreamManualClock
-trait StateDataSourceTestBase extends StreamTest with StateStoreMetricsTest {
+trait StateDataSourceTestBase extends StreamTest with BeforeAndAfter with StateStoreMetricsTest {
Review Comment:
The reason we have to clean up StateStore per test is due to maintenance task. When we run the streaming query, state store is being initialized in to the executor, and registration is performed against the coordinator in driver. The lifecycle of the state store provider is not strictly tied to the the lifecycle of the streaming query - the executor closes the state store provider when coordinator indicates to the executor that the state store provider is no longer valid, which is not immediately after the streaming query has stopped. The lifecycle of the state store provider can overlap among tests.
This means maintenance task against the provider can run after test A. We are clearing the temp directory in test A after the test A has completed, which can break the operation being performed against state store provider being used in test A. E.g. directory no longer exists while maintenance task is running.
This won't be an issue in practice because we do not expect the checkpoint location to be temporary, but it is indeed an issue for how we setup and cleanup env for tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45511][SS] Fix state reader suite flakiness by clean up resources after each test run [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43831:
URL: https://github.com/apache/spark/pull/43831#discussion_r1395351760
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/state/StateDataSourceTestBase.scala:
##########
@@ -18,16 +18,27 @@ package org.apache.spark.sql.execution.datasources.v2.state
import java.sql.Timestamp
+import org.scalatest.BeforeAndAfter
+
import org.apache.spark.sql.{DataFrame, Dataset}
import org.apache.spark.sql.execution.streaming.MemoryStream
+import org.apache.spark.sql.execution.streaming.state.StateStore
import org.apache.spark.sql.functions._
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.streaming._
import org.apache.spark.sql.streaming.util.StreamManualClock
-trait StateDataSourceTestBase extends StreamTest with StateStoreMetricsTest {
+trait StateDataSourceTestBase extends StreamTest with BeforeAndAfter with StateStoreMetricsTest {
Review Comment:
The reason why I requested is that `BeforeAndAfter` is deprecated, @HeartSaVioR . I hope we follow the `scalatest` recommendation in Apache Spark 4.0.0.
- https://www.scalatest.org/scaladoc/1.4.1/org/scalatest/BeforeAndAfter.html
> This trait has been deprecated and will be removed in a future version of ScalaTest. If you are only using its beforeEach and/or afterEach methods, mix in BeforeAndAfterEach instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45511][SS] Fix state reader suite flakiness by clean up resources after each test run [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #43831:
URL: https://github.com/apache/spark/pull/43831#discussion_r1395304223
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/state/StateDataSourceTestBase.scala:
##########
@@ -18,16 +18,27 @@ package org.apache.spark.sql.execution.datasources.v2.state
import java.sql.Timestamp
+import org.scalatest.BeforeAndAfter
+
import org.apache.spark.sql.{DataFrame, Dataset}
import org.apache.spark.sql.execution.streaming.MemoryStream
+import org.apache.spark.sql.execution.streaming.state.StateStore
import org.apache.spark.sql.functions._
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.streaming._
import org.apache.spark.sql.streaming.util.StreamManualClock
-trait StateDataSourceTestBase extends StreamTest with StateStoreMetricsTest {
+trait StateDataSourceTestBase extends StreamTest with BeforeAndAfter with StateStoreMetricsTest {
Review Comment:
The reason we have to clean up StateStore per test is due to maintenance task. When we run the streaming query, state store is being initialized in to the executor, and registration is performed against the coordinator in driver. The lifecycle of the state store provider is not strictly tied to the the lifecycle of the streaming query - the executor closes the state store provider when coordinator indicates to the executor that the state store provider is no longer valid, which is not immediately after the streaming query has stopped. The lifecycle of the state store provider can overlap among tests.
This means maintenance task against the provider can run after test A. We are clearing the temp directory in test A after the test A has completed, which can break the operation being performed against state store provider being used in test A. E.g. directory no longer exists while maintenance task is running.
(It's far more problematic as exception in maintenance task will unload all state store providers which corresponding tasks may run concurrently, leading failures on running queries, or even JVM crash for RocksDB state store provider. That's scary, but it happens seldom/rarely so we can have time to revisit later.)
This won't be an issue in practice because we do not expect the checkpoint location to be temporary, but it is indeed an issue for how we setup and cleanup env for tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45511][SS] Fix state reader suite flakiness by clean up resources after each test run [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR closed pull request #43831: [SPARK-45511][SS] Fix state reader suite flakiness by clean up resources after each test run
URL: https://github.com/apache/spark/pull/43831
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45511][SS] Fix state reader suite flakiness by clean up resources after each test run [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #43831:
URL: https://github.com/apache/spark/pull/43831#discussion_r1395306567
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/state/StateDataSourceTestBase.scala:
##########
@@ -18,16 +18,27 @@ package org.apache.spark.sql.execution.datasources.v2.state
import java.sql.Timestamp
+import org.scalatest.BeforeAndAfter
+
import org.apache.spark.sql.{DataFrame, Dataset}
import org.apache.spark.sql.execution.streaming.MemoryStream
+import org.apache.spark.sql.execution.streaming.state.StateStore
import org.apache.spark.sql.functions._
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.streaming._
import org.apache.spark.sql.streaming.util.StreamManualClock
-trait StateDataSourceTestBase extends StreamTest with StateStoreMetricsTest {
+trait StateDataSourceTestBase extends StreamTest with BeforeAndAfter with StateStoreMetricsTest {
Review Comment:
Arguably we can defer the cleanup of temp directory till VM shutdown or test suite cleanup and move this to afterAll, but we do this for stream-stream join test suite already, so would like to be consistent with existing practice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45511][SS] Fix state reader suite flakiness by clean up resources after each test run [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43831:
URL: https://github.com/apache/spark/pull/43831#discussion_r1395339234
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/state/StateDataSourceTestBase.scala:
##########
@@ -18,16 +18,27 @@ package org.apache.spark.sql.execution.datasources.v2.state
import java.sql.Timestamp
+import org.scalatest.BeforeAndAfter
+
import org.apache.spark.sql.{DataFrame, Dataset}
import org.apache.spark.sql.execution.streaming.MemoryStream
+import org.apache.spark.sql.execution.streaming.state.StateStore
import org.apache.spark.sql.functions._
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.streaming._
import org.apache.spark.sql.streaming.util.StreamManualClock
-trait StateDataSourceTestBase extends StreamTest with StateStoreMetricsTest {
+trait StateDataSourceTestBase extends StreamTest with BeforeAndAfter with StateStoreMetricsTest {
Review Comment:
Thank you. Got it. Then, shall we use `beforeEach` and `afterEach` because `SparkFunSuite` has `BeforeAndAfterEach`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45511][SS] Fix state reader suite flakiness by clean up resources after each test run [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43831:
URL: https://github.com/apache/spark/pull/43831#discussion_r1395292450
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/state/StateDataSourceTestBase.scala:
##########
@@ -18,16 +18,27 @@ package org.apache.spark.sql.execution.datasources.v2.state
import java.sql.Timestamp
+import org.scalatest.BeforeAndAfter
+
import org.apache.spark.sql.{DataFrame, Dataset}
import org.apache.spark.sql.execution.streaming.MemoryStream
+import org.apache.spark.sql.execution.streaming.state.StateStore
import org.apache.spark.sql.functions._
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.streaming._
import org.apache.spark.sql.streaming.util.StreamManualClock
-trait StateDataSourceTestBase extends StreamTest with StateStoreMetricsTest {
+trait StateDataSourceTestBase extends StreamTest with BeforeAndAfter with StateStoreMetricsTest {
Review Comment:
`StreamTest` already has `BeforeAndAfterAll`. Can we use `beforeAll` and `afterAll` instead, @chaoqin-li1123 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45511] Fix state reader suite flakiness by clean up resources after each test run [spark]
Posted by "chaoqin-li1123 (via GitHub)" <gi...@apache.org>.
chaoqin-li1123 commented on PR #43831:
URL: https://github.com/apache/spark/pull/43831#issuecomment-1813830603
@HeartSaVioR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45511][SS] Fix state reader suite flakiness by clean up resources after each test run [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on PR #43831:
URL: https://github.com/apache/spark/pull/43831#issuecomment-1815413516
Thanks! Merging to master.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45511][SS] Fix state reader suite flakiness by clean up resources after each test run [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #43831:
URL: https://github.com/apache/spark/pull/43831#discussion_r1395304223
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/state/StateDataSourceTestBase.scala:
##########
@@ -18,16 +18,27 @@ package org.apache.spark.sql.execution.datasources.v2.state
import java.sql.Timestamp
+import org.scalatest.BeforeAndAfter
+
import org.apache.spark.sql.{DataFrame, Dataset}
import org.apache.spark.sql.execution.streaming.MemoryStream
+import org.apache.spark.sql.execution.streaming.state.StateStore
import org.apache.spark.sql.functions._
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.streaming._
import org.apache.spark.sql.streaming.util.StreamManualClock
-trait StateDataSourceTestBase extends StreamTest with StateStoreMetricsTest {
+trait StateDataSourceTestBase extends StreamTest with BeforeAndAfter with StateStoreMetricsTest {
Review Comment:
The reason we have to clean up StateStore per test is due to maintenance task. When we run the streaming query, state store is being initialized in to the executor, and registration is performed against the coordinator in driver. The lifecycle of the state store provider is not strictly tied to the the lifecycle of the streaming query - the executor closes the state store provider when coordinator indicates to the executor that the state store provider is no longer valid, which is "after" the streaming query has stopped. The lifecycle of the state store provider can overlap among tests.
This means maintenance task against the provider can run after test A. We are clearing the temp directory in test A after the test A has completed, which can break the operation being performed against state store provider being used in test A. E.g. directory does not exist while maintenance task is running.
This won't be an issue in practice because we do not expect the checkpoint location to be temporary, but it is indeed an issue for how we setup and cleanup env for tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45511][SS] Fix state reader suite flakiness by clean up resources after each test run [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #43831:
URL: https://github.com/apache/spark/pull/43831#discussion_r1395351760
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/state/StateDataSourceTestBase.scala:
##########
@@ -18,16 +18,27 @@ package org.apache.spark.sql.execution.datasources.v2.state
import java.sql.Timestamp
+import org.scalatest.BeforeAndAfter
+
import org.apache.spark.sql.{DataFrame, Dataset}
import org.apache.spark.sql.execution.streaming.MemoryStream
+import org.apache.spark.sql.execution.streaming.state.StateStore
import org.apache.spark.sql.functions._
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.streaming._
import org.apache.spark.sql.streaming.util.StreamManualClock
-trait StateDataSourceTestBase extends StreamTest with StateStoreMetricsTest {
+trait StateDataSourceTestBase extends StreamTest with BeforeAndAfter with StateStoreMetricsTest {
Review Comment:
The reason why I requested is that `BeforeAndAfter` is deprecated, @HeartSaVioR . I hope we follow the `scalatest` recommendation in Apache Spark 4.0.0 and avoid adding more instances of this deprecated class.
- https://www.scalatest.org/scaladoc/1.4.1/org/scalatest/BeforeAndAfter.html
> This trait has been deprecated and will be removed in a future version of ScalaTest. If you are only using its beforeEach and/or afterEach methods, mix in BeforeAndAfterEach instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45511][SS] Fix state reader suite flakiness by clean up resources after each test run [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #43831:
URL: https://github.com/apache/spark/pull/43831#discussion_r1395347407
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/state/StateDataSourceTestBase.scala:
##########
@@ -18,16 +18,27 @@ package org.apache.spark.sql.execution.datasources.v2.state
import java.sql.Timestamp
+import org.scalatest.BeforeAndAfter
+
import org.apache.spark.sql.{DataFrame, Dataset}
import org.apache.spark.sql.execution.streaming.MemoryStream
+import org.apache.spark.sql.execution.streaming.state.StateStore
import org.apache.spark.sql.functions._
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.streaming._
import org.apache.spark.sql.streaming.util.StreamManualClock
-trait StateDataSourceTestBase extends StreamTest with StateStoreMetricsTest {
+trait StateDataSourceTestBase extends StreamTest with BeforeAndAfter with StateStoreMetricsTest {
Review Comment:
This follows the pattern we used in the existing suite.
https://github.com/apache/spark/blob/master/sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamingJoinSuite.scala#L42-L54
But I'm fine either way. Good to know we already extend BeforeAndAfterEach by default and no need to extend others.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45511][SS] Fix state reader suite flakiness by clean up resources after each test run [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #43831:
URL: https://github.com/apache/spark/pull/43831#discussion_r1395360527
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/state/StateDataSourceTestBase.scala:
##########
@@ -18,16 +18,27 @@ package org.apache.spark.sql.execution.datasources.v2.state
import java.sql.Timestamp
+import org.scalatest.BeforeAndAfter
+
import org.apache.spark.sql.{DataFrame, Dataset}
import org.apache.spark.sql.execution.streaming.MemoryStream
+import org.apache.spark.sql.execution.streaming.state.StateStore
import org.apache.spark.sql.functions._
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.streaming._
import org.apache.spark.sql.streaming.util.StreamManualClock
-trait StateDataSourceTestBase extends StreamTest with StateStoreMetricsTest {
+trait StateDataSourceTestBase extends StreamTest with BeforeAndAfter with StateStoreMetricsTest {
Review Comment:
Ah, great point. Thanks for the context! @chaoqin-li1123 Shall we update the trait?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45511][SS] Fix state reader suite flakiness by clean up resources after each test run [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on PR #43831:
URL: https://github.com/apache/spark/pull/43831#issuecomment-1815415928
Ah I missed the title - @chaoqin-li1123 let's add `[FOLLOWUP]` tag in the PR title when you add a small change on top of resolved JIRA and want to reuse an already resolved JIRA.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45511][SS] Fix state reader suite flakiness by clean up resources after each test run [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #43831:
URL: https://github.com/apache/spark/pull/43831#discussion_r1395304223
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/state/StateDataSourceTestBase.scala:
##########
@@ -18,16 +18,27 @@ package org.apache.spark.sql.execution.datasources.v2.state
import java.sql.Timestamp
+import org.scalatest.BeforeAndAfter
+
import org.apache.spark.sql.{DataFrame, Dataset}
import org.apache.spark.sql.execution.streaming.MemoryStream
+import org.apache.spark.sql.execution.streaming.state.StateStore
import org.apache.spark.sql.functions._
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.streaming._
import org.apache.spark.sql.streaming.util.StreamManualClock
-trait StateDataSourceTestBase extends StreamTest with StateStoreMetricsTest {
+trait StateDataSourceTestBase extends StreamTest with BeforeAndAfter with StateStoreMetricsTest {
Review Comment:
The reason we have to clean up StateStore per test is due to maintenance task. When we run the streaming query, state store is being initialized in to the executor, and registration is performed against the coordinator in driver. The lifecycle of the state store provider is not strictly tied to the the lifecycle of the streaming query - the executor closes the state store provider when coordinator indicates to the executor that the state store provider is no longer valid, which is not immediately after the streaming query has stopped. The lifecycle of the state store provider can overlap among tests.
This means maintenance task against the provider can run after test A. We are clearing the temp directory in test A after the test A has completed, which can break the operation being performed against state store provider being used in test A. E.g. directory does not exist while maintenance task is running.
This won't be an issue in practice because we do not expect the checkpoint location to be temporary, but it is indeed an issue for how we setup and cleanup env for tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org