Re: [PR] [SPARK-46249][SS] Require instance lock for acquiring RocksDB metrics to prevent race with background operations [spark]

2023-12-04 Thread via GitHub 


HeartSaVioR closed pull request #44165: [SPARK-46249][SS] Require instance lock 
for acquiring RocksDB metrics to prevent race with background operations
URL: https://github.com/apache/spark/pull/44165


-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Re: [PR] [SPARK-46249][SS] Require instance lock for acquiring RocksDB metrics to prevent race with background operations [spark]

2023-12-04 Thread via GitHub 


HeartSaVioR commented on PR #44165:
URL: https://github.com/apache/spark/pull/44165#issuecomment-1840065025

   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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Re: [PR] [SPARK-46249][SS] Require instance lock for acquiring RocksDB metrics to prevent race with background operations [spark]

2023-12-04 Thread via GitHub 


HeartSaVioR commented on PR #44165:
URL: https://github.com/apache/spark/pull/44165#issuecomment-1840064037

   This only failed in docker integration test suite which is unrelated.


-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Re: [PR] [SPARK-46249][SS] Require instance lock for acquiring RocksDB metrics to prevent race with background operations [spark]

2023-12-04 Thread via GitHub 


anishshri-db commented on code in PR #44165:
URL: https://github.com/apache/spark/pull/44165#discussion_r1414826922


##
sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/RocksDBStateStoreSuite.scala:
##
@@ -131,19 +131,21 @@ class RocksDBStateStoreSuite extends 
StateStoreSuiteBase[RocksDBStateStoreProvid
 }
 withSQLConf(SQLConf.STATE_STORE_MIN_DELTAS_FOR_SNAPSHOT.key -> "1") {
   tryWithProviderResource(newStoreProvider()) { provider =>
-  val store = provider.getStore(0)
-  // Verify state after updating
-  put(store, "a", 0, 1)
-  assert(get(store, "a", 0) === Some(1))
-  assert(store.commit() === 1)
-  provider.doMaintenance()
-  assert(store.hasCommitted)
-  val storeMetrics = store.metrics
-  assert(storeMetrics.numKeys === 1)
+val store = provider.getStore(0)
+// Verify state after updating
+put(store, "a", 0, 1)
+assert(get(store, "a", 0) === Some(1))
+assert(store.commit() === 1)
+provider.doMaintenance()
+assert(store.hasCommitted)
+val storeMetrics = store.metrics
+assert(storeMetrics.numKeys === 1)
+if (!isChangelogCheckpointingEnabled) {

Review Comment:
   Done



-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Re: [PR] [SPARK-46249][SS] Require instance lock for acquiring RocksDB metrics to prevent race with background operations [spark]

2023-12-04 Thread via GitHub 


HeartSaVioR commented on code in PR #44165:
URL: https://github.com/apache/spark/pull/44165#discussion_r1414814387


##
sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/RocksDBStateStoreSuite.scala:
##
@@ -131,19 +131,21 @@ class RocksDBStateStoreSuite extends 
StateStoreSuiteBase[RocksDBStateStoreProvid
 }
 withSQLConf(SQLConf.STATE_STORE_MIN_DELTAS_FOR_SNAPSHOT.key -> "1") {
   tryWithProviderResource(newStoreProvider()) { provider =>
-  val store = provider.getStore(0)

Review Comment:
   Nice fix on indentation :)



##
sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/RocksDBStateStoreSuite.scala:
##
@@ -131,19 +131,21 @@ class RocksDBStateStoreSuite extends 
StateStoreSuiteBase[RocksDBStateStoreProvid
 }
 withSQLConf(SQLConf.STATE_STORE_MIN_DELTAS_FOR_SNAPSHOT.key -> "1") {
   tryWithProviderResource(newStoreProvider()) { provider =>
-  val store = provider.getStore(0)
-  // Verify state after updating
-  put(store, "a", 0, 1)
-  assert(get(store, "a", 0) === Some(1))
-  assert(store.commit() === 1)
-  provider.doMaintenance()
-  assert(store.hasCommitted)
-  val storeMetrics = store.metrics
-  assert(storeMetrics.numKeys === 1)
+val store = provider.getStore(0)
+// Verify state after updating
+put(store, "a", 0, 1)
+assert(get(store, "a", 0) === Some(1))
+assert(store.commit() === 1)
+provider.doMaintenance()
+assert(store.hasCommitted)
+val storeMetrics = store.metrics
+assert(storeMetrics.numKeys === 1)
+if (!isChangelogCheckpointingEnabled) {

Review Comment:
   Probably better to leave a code comment for future readers.



-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Re: [PR] [SPARK-46249][SS] Require instance lock for acquiring RocksDB metrics to prevent race with background operations [spark]

2023-12-04 Thread via GitHub 


anishshri-db commented on code in PR #44165:
URL: https://github.com/apache/spark/pull/44165#discussion_r1414787796


##
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/RocksDB.scala:
##
@@ -137,6 +137,10 @@ class RocksDB(
   @volatile private var numKeysOnWritingVersion = 0L
   @volatile private var fileManagerMetrics = 
RocksDBFileManagerMetrics.EMPTY_METRICS
 
+  // SPARK-46249 - Keep track of recorded metrics per version which can be 
used for querying later
+  // Updates and access to recordedMetrics are protected by the DB instance 
lock
+  @volatile private var recordedMetrics: Option[RocksDBMetrics] = None

Review Comment:
   Done



-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Re: [PR] [SPARK-46249][SS] Require instance lock for acquiring RocksDB metrics to prevent race with background operations [spark]

2023-12-04 Thread via GitHub 


HeartSaVioR commented on code in PR #44165:
URL: https://github.com/apache/spark/pull/44165#discussion_r1414783355


##
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/RocksDB.scala:
##
@@ -137,6 +137,10 @@ class RocksDB(
   @volatile private var numKeysOnWritingVersion = 0L
   @volatile private var fileManagerMetrics = 
RocksDBFileManagerMetrics.EMPTY_METRICS
 
+  // SPARK-46249 - Keep track of recorded metrics per version which can be 
used for querying later
+  // Updates and access to recordedMetrics are protected by the DB instance 
lock
+  @volatile private var recordedMetrics: Option[RocksDBMetrics] = None

Review Comment:
   Let's explicitly add annotation for which lock the field depends on, like 
below field.
   
   `@GuardedBy("acquireLock")`



-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Re: [PR] [SPARK-46249][SS] Require instance lock for acquiring RocksDB metrics to prevent race with background operations [spark]

2023-12-04 Thread via GitHub 


anishshri-db commented on PR #44165:
URL: https://github.com/apache/spark/pull/44165#issuecomment-1839490380

   cc - @HeartSaVioR - PTAL, thx !


-- 
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-unsubscr...@spark.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org