You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/09/10 12:22:32 UTC
[GitHub] [spark] daugraph opened a new pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
daugraph opened a new pull request #33957:
URL: https://github.com/apache/spark/pull/33957
### What changes were proposed in this pull request?
Incorrect order of variable initialization may lead to incorrect behavior, Related code: TorrentBroadcast.scala , TorrentBroadCast will get wrong checksumEnabled value after initialization, this may not be what we need, we can move L94 front of setConf(SparkEnv.get.conf) to avoid this.
Supplement:
```scala
class Broadcast {
def setConf(): Unit = {
checksumEnabled = true
}
setConf()
var checksumEnabled = false
}
println(new Broadcast().checksumEnabled)
```
output:
```scala
false
```
```scala
class Broadcast {
var checksumEnabled = false
def setConf(): Unit = {
checksumEnabled = true
}
setConf()
}
println(new Broadcast().checksumEnabled)
```
output:
```scala
true
```
### Why are the changes needed?
we can move L94 front of setConf(SparkEnv.get.conf) to avoid this.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
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
[GitHub] [spark] srowen closed pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
srowen closed pull request #33957:
URL: https://github.com/apache/spark/pull/33957
--
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-916870387
Can one of the admins verify this patch?
--
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
[GitHub] [spark] mridulm commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
mridulm commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-917074116
Ok to 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
[GitHub] [spark] srowen commented on a change in pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #33957:
URL: https://github.com/apache/spark/pull/33957#discussion_r716345513
##########
File path: core/src/main/scala/org/apache/spark/broadcast/TorrentBroadcast.scala
##########
@@ -90,8 +94,6 @@ private[spark] class TorrentBroadcast[T: ClassTag](obj: T, id: Long)
/** Total number of blocks this broadcast variable contains. */
private val numBlocks: Int = writeBlocks(obj)
- /** Whether to generate checksum for blocks or not. */
- private var checksumEnabled: Boolean = false
/** The checksum for all the blocks. */
private var checksums: Array[Int] = _
Review comment:
I think it's just ordering here. If setConf happens before the initializer of foo, and then foo is declared and initialized, the initializer overwrites whatever setConf did
--
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-917123093
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/47657/
--
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
[GitHub] [spark] AmplabJenkins commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-938021091
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/144000/
--
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
[GitHub] [spark] SparkQA removed a comment on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-937871250
**[Test build #144000 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/144000/testReport)** for PR 33957 at commit [`ea52165`](https://github.com/apache/spark/commit/ea521654265b71eb097c9662a5537cbabac90c80).
--
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
[GitHub] [spark] SparkQA commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-937942484
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48468/
--
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
[GitHub] [spark] srowen commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-937863793
--
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
[GitHub] [spark] SparkQA commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-938019532
**[Test build #144000 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/144000/testReport)** for PR 33957 at commit [`ea52165`](https://github.com/apache/spark/commit/ea521654265b71eb097c9662a5537cbabac90c80).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
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
[GitHub] [spark] mridulm commented on a change in pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
mridulm commented on a change in pull request #33957:
URL: https://github.com/apache/spark/pull/33957#discussion_r717148570
##########
File path: core/src/main/scala/org/apache/spark/broadcast/TorrentBroadcast.scala
##########
@@ -90,8 +94,6 @@ private[spark] class TorrentBroadcast[T: ClassTag](obj: T, id: Long)
/** Total number of blocks this broadcast variable contains. */
private val numBlocks: Int = writeBlocks(obj)
- /** Whether to generate checksum for blocks or not. */
- private var checksumEnabled: Boolean = false
/** The checksum for all the blocks. */
private var checksums: Array[Int] = _
Review comment:
To clarify - `checksums` is updated in `writeBlocks` - which happens before the field declaration of `checksums`.
The problem with `writeBlocks` is a variant of the `setConf` issue itself.
The difference between both (and because of which `writeBlocks`/`checksums` is not an issue while `checksumEnabled` is an issue) is whether the initialization is to `_` vs or to an explicit value (as illustrated in the example above).
##########
File path: core/src/main/scala/org/apache/spark/broadcast/TorrentBroadcast.scala
##########
@@ -90,8 +94,6 @@ private[spark] class TorrentBroadcast[T: ClassTag](obj: T, id: Long)
/** Total number of blocks this broadcast variable contains. */
private val numBlocks: Int = writeBlocks(obj)
- /** Whether to generate checksum for blocks or not. */
- private var checksumEnabled: Boolean = false
/** The checksum for all the blocks. */
private var checksums: Array[Int] = _
Review comment:
To clarify - `checksums` is updated in `writeBlocks` - which happens before the field declaration of `checksums`.
The problem with `writeBlocks` is a variant of the `setConf` issue itself.
The difference between both (and because of which `writeBlocks`/`checksums` is not an issue while `checksumEnabled` is an issue) is whether the initialization is to `_` vs to an explicit value (as illustrated in the example above).
--
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
[GitHub] [spark] mridulm commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
mridulm commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-932997929
You could create a broadcast variable and check if `checksums` array is non-null.
Either via reflection or add a `private[broadcast] def getChecksums = checksums` - and validate the array has results.
--
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
[GitHub] [spark] AmplabJenkins commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-917194386
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/143153/
--
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
[GitHub] [spark] AmplabJenkins commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-917123093
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/47657/
--
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
[GitHub] [spark] SparkQA removed a comment on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-917087662
**[Test build #143153 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/143153/testReport)** for PR 33957 at commit [`ea52165`](https://github.com/apache/spark/commit/ea521654265b71eb097c9662a5537cbabac90c80).
--
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-938021091
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/144000/
--
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
[GitHub] [spark] mridulm commented on a change in pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
mridulm commented on a change in pull request #33957:
URL: https://github.com/apache/spark/pull/33957#discussion_r716311556
##########
File path: core/src/main/scala/org/apache/spark/broadcast/TorrentBroadcast.scala
##########
@@ -90,8 +94,6 @@ private[spark] class TorrentBroadcast[T: ClassTag](obj: T, id: Long)
/** Total number of blocks this broadcast variable contains. */
private val numBlocks: Int = writeBlocks(obj)
- /** Whether to generate checksum for blocks or not. */
- private var checksumEnabled: Boolean = false
/** The checksum for all the blocks. */
private var checksums: Array[Int] = _
Review comment:
TL/DR; you are right, no change required for `checksums`.
The difference seems to be a nuance of `_` vs explicit value (`checksums` is initialized in `val nunBlocks = writeBlocks(obj)`, while declared after).
```
scala > class Test {
| var a = test()
| var b: String = "123"
| var c: String = _
| def test(): Int = {
| b = "abc"
| c = "abc"
| 1
| }
| }
scala> val test = new Test()
val test: Test = Test@2184c27b
scala> test.a
val res11: Int = 1
scala> test.b
val res12: String = 123
scala> test.c
val res13: String = abc
```
--
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
[GitHub] [spark] srowen commented on a change in pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #33957:
URL: https://github.com/apache/spark/pull/33957#discussion_r716247475
##########
File path: core/src/main/scala/org/apache/spark/broadcast/TorrentBroadcast.scala
##########
@@ -90,8 +94,6 @@ private[spark] class TorrentBroadcast[T: ClassTag](obj: T, id: Long)
/** Total number of blocks this broadcast variable contains. */
private val numBlocks: Int = writeBlocks(obj)
- /** Whether to generate checksum for blocks or not. */
- private var checksumEnabled: Boolean = false
/** The checksum for all the blocks. */
private var checksums: Array[Int] = _
Review comment:
It doesn't look like checksums is set in setConf
--
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
[GitHub] [spark] srowen commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-938593202
Merged to master/3.2/3.1/3.0
--
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
[GitHub] [spark] daugraph commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
daugraph commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-932881859
I have no idea about how to add the test case, plz give me some advice, thanks
--
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
[GitHub] [spark] SparkQA commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-917119701
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47657/
--
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-917371042
Can one of the admins verify this patch?
--
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
[GitHub] [spark] SparkQA commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-937871250
**[Test build #144000 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/144000/testReport)** for PR 33957 at commit [`ea52165`](https://github.com/apache/spark/commit/ea521654265b71eb097c9662a5537cbabac90c80).
--
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-917160008
--
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
[GitHub] [spark] dongjoon-hyun commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-917483838
BTW, thank you for making a PR, @daugraph .
--
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
[GitHub] [spark] mridulm commented on a change in pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
mridulm commented on a change in pull request #33957:
URL: https://github.com/apache/spark/pull/33957#discussion_r716156043
##########
File path: core/src/main/scala/org/apache/spark/broadcast/TorrentBroadcast.scala
##########
@@ -90,8 +94,6 @@ private[spark] class TorrentBroadcast[T: ClassTag](obj: T, id: Long)
/** Total number of blocks this broadcast variable contains. */
private val numBlocks: Int = writeBlocks(obj)
- /** Whether to generate checksum for blocks or not. */
- private var checksumEnabled: Boolean = false
/** The checksum for all the blocks. */
private var checksums: Array[Int] = _
Review comment:
The same applies to `checksums` also ? A test should help validate.
--
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
[GitHub] [spark] SparkQA commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-937871250
--
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
[GitHub] [spark] AmplabJenkins commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-937987673
--
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
[GitHub] [spark] SparkQA removed a comment on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-937871250
**[Test build #144000 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/144000/testReport)** for PR 33957 at commit [`ea52165`](https://github.com/apache/spark/commit/ea521654265b71eb097c9662a5537cbabac90c80).
--
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-917371042
--
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
[GitHub] [spark] srowen commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
srowen commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-937863793
--
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
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-937987673
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/48468/
--
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
[GitHub] [spark] AmplabJenkins commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-916870387
Can one of the admins verify this patch?
--
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
[GitHub] [spark] AmplabJenkins commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-917371042
Can one of the admins verify this patch?
--
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
[GitHub] [spark] mridulm commented on a change in pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
mridulm commented on a change in pull request #33957:
URL: https://github.com/apache/spark/pull/33957#discussion_r716156043
##########
File path: core/src/main/scala/org/apache/spark/broadcast/TorrentBroadcast.scala
##########
@@ -90,8 +94,6 @@ private[spark] class TorrentBroadcast[T: ClassTag](obj: T, id: Long)
/** Total number of blocks this broadcast variable contains. */
private val numBlocks: Int = writeBlocks(obj)
- /** Whether to generate checksum for blocks or not. */
- private var checksumEnabled: Boolean = false
/** The checksum for all the blocks. */
private var checksums: Array[Int] = _
Review comment:
The same applies to `checksums` also ?
--
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
[GitHub] [spark] mridulm commented on a change in pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
mridulm commented on a change in pull request #33957:
URL: https://github.com/apache/spark/pull/33957#discussion_r717148570
##########
File path: core/src/main/scala/org/apache/spark/broadcast/TorrentBroadcast.scala
##########
@@ -90,8 +94,6 @@ private[spark] class TorrentBroadcast[T: ClassTag](obj: T, id: Long)
/** Total number of blocks this broadcast variable contains. */
private val numBlocks: Int = writeBlocks(obj)
- /** Whether to generate checksum for blocks or not. */
- private var checksumEnabled: Boolean = false
/** The checksum for all the blocks. */
private var checksums: Array[Int] = _
Review comment:
To clarify - `checksums` is updated in `writeBlocks` - which happens before the field declaration of `checksums`.
The problem with `writeBlocks` is a variant of the `setConf` issue itself.
The difference between both (and because of which `writeBlocks`/`checksums` is not an issue while `checksumEnabled` is an issue) is whether the initialization is to `_` vs or to an explicit value (as illustrated in the example above).
--
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
[GitHub] [spark] HyukjinKwon commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-938243058
Yeah, I am okie too.
--
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
[GitHub] [spark] mridulm commented on a change in pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
mridulm commented on a change in pull request #33957:
URL: https://github.com/apache/spark/pull/33957#discussion_r717148570
##########
File path: core/src/main/scala/org/apache/spark/broadcast/TorrentBroadcast.scala
##########
@@ -90,8 +94,6 @@ private[spark] class TorrentBroadcast[T: ClassTag](obj: T, id: Long)
/** Total number of blocks this broadcast variable contains. */
private val numBlocks: Int = writeBlocks(obj)
- /** Whether to generate checksum for blocks or not. */
- private var checksumEnabled: Boolean = false
/** The checksum for all the blocks. */
private var checksums: Array[Int] = _
Review comment:
To clarify - `checksums` is updated in `writeBlocks` - which happens before the field declaration of `checksums`.
The problem with `writeBlocks` is a variant of the `setConf` issue itself.
The difference between both (and because of which `writeBlocks`/`checksums` is not an issue while `checksumEnabled` is an issue) is whether the initialization is to `_` vs to an explicit value (as illustrated in the example above).
--
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
[GitHub] [spark] AmplabJenkins commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-937987673
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/48468/
--
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
[GitHub] [spark] SparkQA commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-937987648
Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/48468/
--
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
[GitHub] [spark] SparkQA commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-917182972
**[Test build #143153 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/143153/testReport)** for PR 33957 at commit [`ea52165`](https://github.com/apache/spark/commit/ea521654265b71eb097c9662a5537cbabac90c80).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
--
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
[GitHub] [spark] AmplabJenkins commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-917160008
Can one of the admins verify this patch?
--
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
[GitHub] [spark] SparkQA commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-917114193
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/47657/
--
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
[GitHub] [spark] SparkQA commented on pull request #33957: [SPARK-36717][CORE] Incorrect order of variable initialization may lead incorrect behavior
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33957:
URL: https://github.com/apache/spark/pull/33957#issuecomment-917087662
**[Test build #143153 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/143153/testReport)** for PR 33957 at commit [`ea52165`](https://github.com/apache/spark/commit/ea521654265b71eb097c9662a5537cbabac90c80).
--
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