Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Improvement-16507] Remove spring-boot-starter-cache #16593

Merged
merged 49 commits into from
Oct 21, 2024
Merged

Conversation

binitshrest
Copy link
Contributor

@binitshrest binitshrest commented Sep 5, 2024

I changed and removed the some extra dependencies that should be removed after removing spring-boot-starter-cache from pom.xml and application.properties file. That has caused the issues. After removing extra dependencies and the server runs smoothly with out any bugs

close #16507

Copy link

boring-cyborg bot commented Sep 5, 2024

Thanks for opening this pull request! Please check out our contributing guidelines. (https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md)

@binitshrest binitshrest changed the title [Improvement-16507][alert] Improve the performance of spring-boot-starter-cache [Improvement-16507][Remove spring-boot-starter-cache dependency] Improve the performance of spring-boot-starter-cache Sep 5, 2024
@binitshrest
Copy link
Contributor Author

Thanks for opening this pull request! Please check out our contributing guidelines. (https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md)
Hi, Can you please review if i updated based on the given documentations.

Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

We should remove the code and dependency of spring-boot-starter-cache.

@binitshrest
Copy link
Contributor Author

We should remove the code and dependency of spring-boot-starter-cache.

Hi ruanwenjun,
After carefully observing and understanding the codebase. I have removed the extra dependency of the spring-boot-starter-cache which includes the related annotations like cacheable and cacheconfig. I removed it and the codebase runs without any issues. I am open for some more suggestions from your side. As spring-boot-starter-cache dependency has been removed from POM.xml and application.properties files. I went some more depth and removed some related annotations.

@binitshrest
Copy link
Contributor Author

@caishunfeng Can you please guide me to fix 5 failing issues?

@SbloodyS
Copy link
Member

SbloodyS commented Sep 9, 2024

You should check these two dependency. @binitshrest

log4j-api-2.17.2.jar
log4j-to-slf4j-2.17.2.jar

@binitshrest
Copy link
Contributor Author

You should check these two dependency. @binitshrest

log4j-api-2.17.2.jar
log4j-to-slf4j-2.17.2.jar

log4j-to-slf4j-2.17.2.jar

Thank you SbloodyS. I found them. There are so many of these dependencies. Should i remove them too.

@binitshrest
Copy link
Contributor Author

You should check these two dependency. @binitshrest

log4j-api-2.17.2.jar
log4j-to-slf4j-2.17.2.jar

log4j-to-slf4j-2.17.2.jar

Thank you @SbloodyS. I found them. There are so many of these dependencies. Should i remove them too.

@ruanwenjun
Copy link
Member

ruanwenjun commented Oct 14, 2024

@ruanwenjun sir. Did i solve this issue? Please let me know. I am encouraged that all the checks are passed.

@binitshrest Yes, the ci has passed. But there are some extra things need to do.

  1. Please remove two license files
`dolphinscheduler-dist/release-docs/licenses/LICENSE-log4j-api-2.11.2.txt`
`dolphinscheduler-dist/release-docs/licenses/LICENSE-log4j-core-2.11.2.txt`

Since you have removed these from LICENSE, and we really don't rely on these, good job, you have solved a new problem.
2. I am not sure if we need to add exclusion in all place, I add comment. Since once we add these, it will be hard to remove in the future, my experience is we only need to exclude these in dependencymanagement. It's need to have a try.

@binitshrest
Copy link
Contributor Author

@ruanwenjun sir. Did i solve this issue? Please let me know. I am encouraged that all the checks are passed.

@binitshrest Yes, the ci has passed. But there are some extra things need to do.

  1. Please remove two license files
`dolphinscheduler-dist/release-docs/licenses/LICENSE-log4j-api-2.11.2.txt`
`dolphinscheduler-dist/release-docs/licenses/LICENSE-log4j-core-2.11.2.txt`

Since you have removed these from LICENSE, and we really don't rely on these, good job, you have solved a new problem. 2. I am not sure if we need to add exclusion in all place, I add comment. Since once we add these, it will be hard to remove in the future, my experience is we only need to exclude these in dependencymanagement. It's need to have a try.

Yes, Sir. We don't need to add that exclusion code in all place. I saw in this morning. Once i put that exclusion in root dependency. It will remove from all. I am not sure if i should remove from test scope . I will make also some changes what you have said earlier.

@binitshrest
Copy link
Contributor Author

Please retry analysis of this Pull-Request directly on SonarCloud

Do i have to worry about this sir? @ruanwenjun

@ruanwenjun
Copy link
Member

Please retry analysis of this Pull-Request directly on SonarCloud

Do i have to worry about this sir? @ruanwenjun

No, this is not related to your change, and will not block this PR.

ruanwenjun
ruanwenjun previously approved these changes Oct 15, 2024
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

Generally LGTM. Just some NIT.

.gitignore Outdated
@@ -54,4 +54,4 @@ dolphinscheduler-worker/logs
dolphinscheduler-master/logs
dolphinscheduler-api/logs
__pycache__
ds_schema_check_test
ds_schema_check_test
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid unnessnary chanage.

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 still unaddressed.

@binitshrest
Copy link
Contributor Author

This issue was closed accidentally. I am reopening this issue.

@binitshrest
Copy link
Contributor Author

This issue was closed accidentally. I am reopening this issue.

Reopening

Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

+1

@SbloodyS SbloodyS added the improvement make more easy to user or prompt friendly label Oct 21, 2024
Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

+1

Copy link

sonarcloud bot commented Oct 21, 2024

@SbloodyS SbloodyS merged commit 54a31a7 into apache:dev Oct 21, 2024
68 of 70 checks passed
Copy link

boring-cyborg bot commented Oct 21, 2024

Awesome work, congrats on your first merged pull request!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend first time contributor First-time contributor improvement make more easy to user or prompt friendly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Remove spring-boot-starter-cache dependency
3 participants