Skip to content

Conversation

@panbingkun
Copy link
Contributor

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.


protected val jobConfCacheKey: String = "rdd_%d_job_conf".format(id)

protected val inputFormatCacheKey: String = "rdd_%d_input_format".format(id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a part of developer API. I don't think we can just remove this out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, Let's reserve it.

@HyukjinKwon
Copy link
Member

@panbingkun how did you find them?

FWIW, I thought we added a compilation feature to disallow unused variables IIRC, @LuciferYang .

@panbingkun
Copy link
Contributor Author

@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:
image

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
image

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.
image

6.Some inspect results may be inaccurate and ultimately require manual reconfirmation.

@panbingkun panbingkun requested a review from HyukjinKwon May 19, 2023 01:11
@LuciferYang
Copy link
Contributor

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).

spark/pom.xml

Line 2864 in 411bcd2

<arg>-Ywarn-unused:imports</arg>

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.

(Math.min(totalGCTime / totalDuration + 0.5, 1)) : 1;
}

// When GCTimePercent is edited change ToolTips.TASK_TIME to match
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, why do you propose to remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ToolTips.TASK_TIME - Shaded red when garbage collection (GC) time is over 10% of task time

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"

@panbingkun panbingkun requested a review from MaxGekk May 26, 2023 03:08
@MaxGekk
Copy link
Member

MaxGekk commented May 26, 2023

+1, LGTM. Merging to master.
Thank you, @panbingkun and @LuciferYang @HyukjinKwon for review.

@MaxGekk MaxGekk closed this in 82bf3fc May 26, 2023
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM.

czxm pushed a commit to czxm/spark that referenced this pull request Jun 12, 2023
### 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.

Closes apache#41218 from panbingkun/remove_unused_declaration_core.

Authored-by: panbingkun <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants