You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "panbingkun (via GitHub)" <gi...@apache.org> on 2023/05/18 11:58:39 UTC
[GitHub] [spark] panbingkun opened a new pull request, #41218: [SPARK-43576][CORE] Remove unused declarations from Core module
panbingkun opened a new pull request, #41218:
URL: https://github.com/apache/spark/pull/41218
### What changes were proposed in this pull request?
The pr aims to remove unused declarations from `Core` module
### Why are the changes needed?
Make code clean.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Pass GA.
--
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] LuciferYang commented on pull request #41218: [SPARK-43576][CORE] Remove unused declarations from Core module
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #41218:
URL: https://github.com/apache/spark/pull/41218#issuecomment-1553924881
> IIRC
Currently, we have only added compilation checks on `-Ywarn-unused:imports` for Scala 2.12 (the behavior of Scala 2.13 is slightly different).
https://github.com/apache/spark/blob/411bcd2f18b2275466edc752c8cdcdfcaab1cb9c/pom.xml#L2864
For the current version of Scala 2.12, we can further try to add more checks, such as `patvars`, `privates`, and `locals`. For codes that cannot be changed, we can use `@nowarn` to suppress them.
--
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] MaxGekk commented on pull request #41218: [SPARK-43576][CORE] Remove unused declarations from Core module
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #41218:
URL: https://github.com/apache/spark/pull/41218#issuecomment-1563920852
+1, LGTM. Merging to master.
Thank you, @panbingkun and @LuciferYang @HyukjinKwon for review.
--
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] panbingkun commented on a diff in pull request #41218: [SPARK-43576][CORE] Remove unused declarations from Core module
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #41218:
URL: https://github.com/apache/spark/pull/41218#discussion_r1206196456
##########
core/src/main/resources/org/apache/spark/ui/static/executorspage.js:
##########
@@ -126,7 +126,6 @@ function totalDurationAlpha(totalGCTime, totalDuration) {
(Math.min(totalGCTime / totalDuration + 0.5, 1)) : 1;
}
-// When GCTimePercent is edited change ToolTips.TASK_TIME to match
Review Comment:
ToolTips.TASK_TIME - Shaded red when garbage collection (GC) time is over 10% of task time
- Adding this variable for the first time
CommitID: c037d25482ea63430fb42bfd86124c268be5a4a4
JIRA: https://issues.apache.org/jira/browse/SPARK-12149
[SPARK-12149][WEB UI] Executor UI improvement suggestions - Color UI
PR: https://github.com/apache/spark/pull/10154
First declaration：https://github.com/apache/spark/pull/10154/files#diff-91f8d288f7aa0726fc067ad9eb67aff41c474ada4d702e5b393bd81ae5aee7b5R91-R92
First use：https://github.com/apache/spark/pull/10154/files#diff-3bb7f9962a6343cbc09be474b85f2f9ed2e80047f4863d447faa407d925f425cR82
- The variable content is directly hard coding in `executorspage-template.html` file
CommitID: b9bab4dcf6cec5ec9e9860871f9dd1ec633a1d22
JIRA: https://issues.apache.org/jira/browse/SPARK-15951
[SPARK-15951] Change Executors Page to use datatables to support sorting columns and searching
PR: https://github.com/apache/spark/pull/13670
The variable content is directly hard coding: https://github.com/apache/spark/pull/13670/files#diff-cc6993f75c03b53bb41d8d7113ed3d5157580e583672da68b15cbe4d9f374e15R36
Delete: https://github.com/apache/spark/pull/13670/files#diff-3bb7f9962a6343cbc09be474b85f2f9ed2e80047f4863d447faa407d925f425cL94
**So I think it needs to be removed; Meanwhile, I did not find any usage in the `spark` code base using the following command: find . -name "*" -type file -print0 | xargs -0 grep "TASK_TIME"**
--
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] MaxGekk commented on a diff in pull request #41218: [SPARK-43576][CORE] Remove unused declarations from Core module
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #41218:
URL: https://github.com/apache/spark/pull/41218#discussion_r1205456163
##########
core/src/main/resources/org/apache/spark/ui/static/executorspage.js:
##########
@@ -126,7 +126,6 @@ function totalDurationAlpha(totalGCTime, totalDuration) {
(Math.min(totalGCTime / totalDuration + 0.5, 1)) : 1;
}
-// When GCTimePercent is edited change ToolTips.TASK_TIME to match
Review Comment:
hmm, why do you propose to remove this?
--
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] MaxGekk closed pull request #41218: [SPARK-43576][CORE] Remove unused declarations from Core module
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #41218: [SPARK-43576][CORE] Remove unused declarations from Core module
URL: https://github.com/apache/spark/pull/41218
--
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 #41218: [SPARK-43576][CORE] Remove unused declarations from Core module
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #41218:
URL: https://github.com/apache/spark/pull/41218#issuecomment-1553807658
@panbingkun how did you find them?
FWIW, I thought we added a compilation feature to disallow unused variables IIRC, @LuciferYang .
--
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] panbingkun commented on a diff in pull request #41218: [SPARK-43576][CORE] Remove unused declarations from Core module
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on code in PR #41218:
URL: https://github.com/apache/spark/pull/41218#discussion_r1198432812
##########
core/src/main/scala/org/apache/spark/rdd/HadoopRDD.scala:
##########
@@ -128,8 +128,6 @@ class HadoopRDD[K, V](
protected val jobConfCacheKey: String = "rdd_%d_job_conf".format(id)
- protected val inputFormatCacheKey: String = "rdd_%d_input_format".format(id)
Review Comment:
Ok, Let's reserve 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
[GitHub] [spark] panbingkun commented on pull request #41218: [SPARK-43576][CORE] Remove unused declarations from Core module
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
panbingkun commented on PR #41218:
URL: https://github.com/apache/spark/pull/41218#issuecomment-1553870280
> @panbingkun how did you find them?
>
> FWIW, I thought we added a compilation feature to disallow unused variables IIRC, @LuciferYang .
@HyukjinKwon
1.When I review the HadoopRDD code, I noticed some inspection prompts, eg:
<img width="808" alt="image" src="https://github.com/apache/spark/assets/15246973/4dc7c9b9-a549-4e32-a0cf-679a236f13b1">
2.So I wonder if there are other cases in the core module, then I export `unused declaration` rule to single file, name as `unused_declaration.xml`
<img width="443" alt="image" src="https://github.com/apache/spark/assets/15246973/be2d73d6-a900-4a33-9927-91f9e191703c">
3.execute command:
"/Users/panbingkun/Library/Application Support/JetBrains/Toolbox/apps/IDEA-C/ch-0/231.9011.34/IntelliJ IDEA CE.app/Contents/bin/inspect.sh" /Users/panbingkun/Developer/spark/spark-community /Users/panbingkun/Developer/spark/spark-community/unused_declaration.xml /Users/panbingkun/Developer/spark/spark-community/unused_declaration_results -v2 -d /Users/panbingkun/Developer/spark/spark-community/core/src
4.The declared unused variable results in all `core` modules will be saved in the `unused_declaration_results` file.
5.Open it by IDE.
<img width="695" alt="image" src="https://github.com/apache/spark/assets/15246973/74096512-4f09-4fab-8678-97e6a59cd85c">
6.Some inspect results may be inaccurate and ultimately require manual reconfirmation.
--
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 a diff in pull request #41218: [SPARK-43576][CORE] Remove unused declarations from Core module
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #41218:
URL: https://github.com/apache/spark/pull/41218#discussion_r1198402967
##########
core/src/main/scala/org/apache/spark/rdd/HadoopRDD.scala:
##########
@@ -128,8 +128,6 @@ class HadoopRDD[K, V](
protected val jobConfCacheKey: String = "rdd_%d_job_conf".format(id)
- protected val inputFormatCacheKey: String = "rdd_%d_input_format".format(id)
Review Comment:
This is a part of developer API. I don't think we can just remove this out.
--
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