You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "tedyu (via GitHub)" <gi...@apache.org> on 2024/12/06 10:38:22 UTC
[PR] [MINOR] Assign null to input after closing it [spark]
tedyu opened a new pull request, #49091:
URL: https://github.com/apache/spark/pull/49091
### What changes were proposed in this pull request?
This is followup to https://github.com/apache/spark/pull/48880
After `input` is closed, its value is changed to `null`.
`readVersion` is refactored accordingly to return the previously parsed version.
### Why are the changes needed?
It is good practice to nullify `input` after closing it.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Existing test suite.
### 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] [MINOR] Assign null to input after closing it [spark]
Posted by "tedyu (via GitHub)" <gi...@apache.org>.
tedyu commented on PR #49091:
URL: https://github.com/apache/spark/pull/49091#issuecomment-2523303355
cc @WweiL @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-50360] [Follow-up] [MINOR] Make readVersion lazy value [spark]
Posted by "tedyu (via GitHub)" <gi...@apache.org>.
tedyu commented on code in PR #49091:
URL: https://github.com/apache/spark/pull/49091#discussion_r1884724577
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStoreChangelog.scala:
##########
@@ -353,7 +353,7 @@ class StateStoreChangelogReaderFactory(
}
protected val input: DataInputStream = decompressStream(sourceStream)
- private def readVersion(): Short = {
+ private lazy val readVersion: Short = {
Review Comment:
the current naming leaves no room for ambiguity.
--
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-50360] [Follow-up] [MINOR] Make readVersion lazy value [spark]
Posted by "tedyu (via GitHub)" <gi...@apache.org>.
tedyu commented on PR #49091:
URL: https://github.com/apache/spark/pull/49091#issuecomment-2542746467
@HeartSaVioR
Please take another look.
Test failure was not caused by the PR.
--
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] [Follow-up] [MINOR] Make readVersion lazy value [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #49091:
URL: https://github.com/apache/spark/pull/49091#discussion_r1884717486
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStoreChangelog.scala:
##########
@@ -353,7 +353,7 @@ class StateStoreChangelogReaderFactory(
}
protected val input: DataInputStream = decompressStream(sourceStream)
- private def readVersion(): Short = {
+ private lazy val readVersion: Short = {
Review Comment:
nit: it's no longer a method but a value; so probably `version` is the better name?
--
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] [Follow-up] [MINOR] Make readVersion lazy value [spark]
Posted by "WweiL (via GitHub)" <gi...@apache.org>.
WweiL commented on PR #49091:
URL: https://github.com/apache/spark/pull/49091#issuecomment-2542660012
I think it should be renamed as
```
[SPARK-50360][FOLLOWUP][MINOR]Make readVersion lazy value
```
--
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] [Follow-up] [MINOR] Make readVersion lazy value [spark]
Posted by "tedyu (via GitHub)" <gi...@apache.org>.
tedyu commented on PR #49091:
URL: https://github.com/apache/spark/pull/49091#issuecomment-2542537147
@HeartSaVioR
Can you review this PR ?
--
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-50360][SS][FOLLOWUP][MINOR] Make readVersion lazy value [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on PR #49091:
URL: https://github.com/apache/spark/pull/49091#issuecomment-2544492924
The failure only happened with TPC, which isn't relevant.
--
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-50360][SS][FOLLOWUP][MINOR] Make readVersion lazy value [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on PR #49091:
URL: https://github.com/apache/spark/pull/49091#issuecomment-2544494437
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-50360][SS][FOLLOWUP][MINOR] Make readVersion lazy value [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on PR #49091:
URL: https://github.com/apache/spark/pull/49091#issuecomment-2544495864
Forgot to call out +1
--
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] [Follow-up] [MINOR] Make readVersion lazy value [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR commented on code in PR #49091:
URL: https://github.com/apache/spark/pull/49091#discussion_r1884722928
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStoreChangelog.scala:
##########
@@ -353,7 +353,7 @@ class StateStoreChangelogReaderFactory(
}
protected val input: DataInputStream = decompressStream(sourceStream)
- private def readVersion(): Short = {
+ private lazy val readVersion: Short = {
Review Comment:
I feel like the part "changeLog" is redundant because this only deals with changelog, but not a huge deal.
--
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] [Follow-up] [MINOR] Make readVersion lazy value [spark]
Posted by "tedyu (via GitHub)" <gi...@apache.org>.
tedyu commented on code in PR #49091:
URL: https://github.com/apache/spark/pull/49091#discussion_r1884720728
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStoreChangelog.scala:
##########
@@ -353,7 +353,7 @@ class StateStoreChangelogReaderFactory(
}
protected val input: DataInputStream = decompressStream(sourceStream)
- private def readVersion(): Short = {
+ private lazy val readVersion: Short = {
Review Comment:
I renamed it `changeLogVersion`
--
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-50360][SS][FOLLOWUP][MINOR] Make readVersion lazy value [spark]
Posted by "HeartSaVioR (via GitHub)" <gi...@apache.org>.
HeartSaVioR closed pull request #49091: [SPARK-50360][SS][FOLLOWUP][MINOR] Make readVersion lazy value
URL: https://github.com/apache/spark/pull/49091
--
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] [MINOR] Assign null to input after closing it [spark]
Posted by "WweiL (via GitHub)" <gi...@apache.org>.
WweiL commented on code in PR #49091:
URL: https://github.com/apache/spark/pull/49091#discussion_r1884561727
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStoreChangelog.scala:
##########
@@ -351,24 +351,31 @@ class StateStoreChangelogReaderFactory(
case f: FileNotFoundException =>
throw QueryExecutionErrors.failedToReadStreamingStateFileError(fileToRead, f)
}
- protected val input: DataInputStream = decompressStream(sourceStream)
+ protected var input: DataInputStream = decompressStream(sourceStream)
+ protected var version: Short = -1
private def readVersion(): Short = {
Review Comment:
Thanks for the follow up!
Can we just remove this `protected var version: Short = -1` and change this line to
`private lazy val readVersion: Short = {`
--
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