You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "beliefer (via GitHub)" <gi...@apache.org> on 2024/01/27 05:54:44 UTC
[PR] [SPARK-46882][SS][TEST] Replace unnecessary AtomicInteger with int [spark]
beliefer opened a new pull request, #44907:
URL: https://github.com/apache/spark/pull/44907
### What changes were proposed in this pull request?
This PR propose to replace unnecessary `AtomicInteger` with int.
### Why are the changes needed?
The variable `value` of `GetMaxCounter` always guarded by itself. So we can replace the unnecessary `AtomicInteger` with int.
### Does this PR introduce _any_ user-facing change?
'No'.
### How was this patch tested?
GA.
### Was this patch authored or co-authored using generative AI tooling?
'No'.
--
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-46882][SS][TEST] Replace unnecessary AtomicInteger with int [spark]
Posted by "srowen (via GitHub)" <gi...@apache.org>.
srowen commented on code in PR #44907:
URL: https://github.com/apache/spark/pull/44907#discussion_r1471343646
##########
streaming/src/test/scala/org/apache/spark/streaming/util/WriteAheadLogSuite.scala:
##########
@@ -238,14 +237,14 @@ class FileBasedWriteAheadLogSuite
val executionContext = ExecutionContext.fromExecutorService(fpool)
class GetMaxCounter {
- private val value = new AtomicInteger()
+ private var value = 0
@volatile private var max: Int = 0
def increment(): Unit = synchronized {
- val atInstant = value.incrementAndGet()
+ val atInstant = value + 1
if (atInstant > max) max = atInstant
}
- def decrement(): Unit = synchronized { value.decrementAndGet() }
- def get(): Int = synchronized { value.get() }
+ def decrement(): Unit = synchronized { value = value - 1 }
+ def get(): Int = synchronized { value }
def getMax(): Int = synchronized { max }
Review Comment:
I can't see why this change would be different. But there is probably some subtle issue with memory barriers or whatever. I don't think it's worth changing.
--
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-46882][SS][TEST] Replace unnecessary AtomicInteger with int [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #44907:
URL: https://github.com/apache/spark/pull/44907#issuecomment-1920341297
Thank you, @beliefer and all.
--
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-46882][SS][TEST] Replace unnecessary AtomicInteger with int [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44907:
URL: https://github.com/apache/spark/pull/44907#issuecomment-1914190488
cc @dongjoon-hyun @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-46882][SS][TEST] Replace unnecessary AtomicInteger with int [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #44907:
URL: https://github.com/apache/spark/pull/44907#discussion_r1470639046
##########
streaming/src/test/scala/org/apache/spark/streaming/util/WriteAheadLogSuite.scala:
##########
@@ -238,14 +237,14 @@ class FileBasedWriteAheadLogSuite
val executionContext = ExecutionContext.fromExecutorService(fpool)
class GetMaxCounter {
- private val value = new AtomicInteger()
+ private var value = 0
@volatile private var max: Int = 0
def increment(): Unit = synchronized {
- val atInstant = value.incrementAndGet()
+ val atInstant = value + 1
if (atInstant > max) max = atInstant
}
- def decrement(): Unit = synchronized { value.decrementAndGet() }
- def get(): Int = synchronized { value.get() }
+ def decrement(): Unit = synchronized { value = value - 1 }
+ def get(): Int = synchronized { value }
def getMax(): Int = synchronized { max }
Review Comment:
Actually, this suite failed in the CI with your PR, @beliefer .
```
[info] *** 1 TEST FAILED ***
[error] Failed: Total 440, Failed 1, Errors 0, Passed 439, Ignored 1
[error] Failed tests:
[error] org.apache.spark.streaming.util.FileBasedWriteAheadLogSuite
```
--
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-46882][SS][TEST] Replace unnecessary AtomicInteger with int [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44907:
URL: https://github.com/apache/spark/pull/44907#discussion_r1470644716
##########
streaming/src/test/scala/org/apache/spark/streaming/util/WriteAheadLogSuite.scala:
##########
@@ -238,14 +237,14 @@ class FileBasedWriteAheadLogSuite
val executionContext = ExecutionContext.fromExecutorService(fpool)
class GetMaxCounter {
- private val value = new AtomicInteger()
+ private var value = 0
@volatile private var max: Int = 0
def increment(): Unit = synchronized {
- val atInstant = value.incrementAndGet()
+ val atInstant = value + 1
if (atInstant > max) max = atInstant
}
- def decrement(): Unit = synchronized { value.decrementAndGet() }
- def get(): Int = synchronized { value.get() }
+ def decrement(): Unit = synchronized { value = value - 1 }
+ def get(): Int = synchronized { value }
def getMax(): Int = synchronized { max }
Review Comment:
Thank you for your reminder. I will check it.
--
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-46882][SS][TEST] Replace unnecessary AtomicInteger with int [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #44907:
URL: https://github.com/apache/spark/pull/44907#discussion_r1470639865
##########
streaming/src/test/scala/org/apache/spark/streaming/util/WriteAheadLogSuite.scala:
##########
@@ -238,14 +237,14 @@ class FileBasedWriteAheadLogSuite
val executionContext = ExecutionContext.fromExecutorService(fpool)
class GetMaxCounter {
- private val value = new AtomicInteger()
+ private var value = 0
@volatile private var max: Int = 0
def increment(): Unit = synchronized {
- val atInstant = value.incrementAndGet()
+ val atInstant = value + 1
if (atInstant > max) max = atInstant
}
- def decrement(): Unit = synchronized { value.decrementAndGet() }
- def get(): Int = synchronized { value.get() }
+ def decrement(): Unit = synchronized { value = value - 1 }
+ def get(): Int = synchronized { value }
def getMax(): Int = synchronized { max }
Review Comment:
Given the failure, I'm not sure if this test suite change is worthy or not.
##########
streaming/src/test/scala/org/apache/spark/streaming/util/WriteAheadLogSuite.scala:
##########
@@ -238,14 +237,14 @@ class FileBasedWriteAheadLogSuite
val executionContext = ExecutionContext.fromExecutorService(fpool)
class GetMaxCounter {
- private val value = new AtomicInteger()
+ private var value = 0
@volatile private var max: Int = 0
def increment(): Unit = synchronized {
- val atInstant = value.incrementAndGet()
+ val atInstant = value + 1
if (atInstant > max) max = atInstant
}
- def decrement(): Unit = synchronized { value.decrementAndGet() }
- def get(): Int = synchronized { value.get() }
+ def decrement(): Unit = synchronized { value = value - 1 }
+ def get(): Int = synchronized { value }
def getMax(): Int = synchronized { max }
Review Comment:
Given the above failure, I'm not sure if this test suite change is worthy or not.
--
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-46882][SS][TEST] Replace unnecessary AtomicInteger with int [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #44907: [SPARK-46882][SS][TEST] Replace unnecessary AtomicInteger with int
URL: https://github.com/apache/spark/pull/44907
--
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-46882][SS][TEST] Replace unnecessary AtomicInteger with int [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44907:
URL: https://github.com/apache/spark/pull/44907#issuecomment-1913083577
cc @mridulm
--
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-46882][SS][TEST] Replace unnecessary AtomicInteger with int [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44907:
URL: https://github.com/apache/spark/pull/44907#issuecomment-1920353012
@dongjoon-hyun @srowen @mridulm Thank you!
--
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-46882][SS][TEST] Replace unnecessary AtomicInteger with int [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44907:
URL: https://github.com/apache/spark/pull/44907#issuecomment-1920336496
cc @srowen @dongjoon-hyun Although this change is just related to test suite, the cost of AtomicInteger is also higher than int.
--
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