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

[#4520] Imporvement(jdbc-catalogs): Shrink JDBC catalog binary package size to about 3MB. #4521

Merged
merged 5 commits into from
Aug 26, 2024

Conversation

yuqi1129
Copy link
Contributor

@yuqi1129 yuqi1129 commented Aug 14, 2024

What changes were proposed in this pull request?

Remove some unnecessary jar dependencies in the runtime classpath for JDBC catalogs.

Why are the changes needed?

Shrink JDBC catalog package.

Fix: #4520

Does this PR introduce any user-facing change?

N/A.

How was this patch tested?

Test locally and CI.

MySQL catalog:
image

PostgreSQL catalog:

image Doris catalog:

image

@yuqi1129 yuqi1129 changed the title [#4520] Imporvement(jdbc-catalogs): Shrink JDBC catalog binary package size [#4520] Imporvement(jdbc-catalogs): Shrink JDBC catalog binary package size to about 9MB. Aug 15, 2024
@yuqi1129 yuqi1129 changed the title [#4520] Imporvement(jdbc-catalogs): Shrink JDBC catalog binary package size to about 9MB. [#4520] Imporvement(jdbc-catalogs): Shrink JDBC catalog binary package size to about 3MB. Aug 16, 2024
exclude("guava-*.jar")
exclude("log4j-*.jar")
exclude("slf4j-*.jar")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to remove these jars explicitly? If we're using them only in compile, maybe we can change from "implementation" to "compileOnly"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those jars already exist in the server class path, duplicated jars are useless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we're using them only in compile, maybe we can change from "implementation" to "compileOnly"?

Yeah, let me have a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we're using them only in compile, maybe we can change from "implementation" to "compileOnly"?

Yeah, let me have a try.

It seems that making it compileOnly can't pass tests and runs into classes not found problem.

}
implementation(project(":catalogs:catalog-common")) {
exclude(group = "*")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why only mysql relies on "catalog-common"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was added by @FANNG1 , let me check the reason.

Copy link
Contributor Author

@yuqi1129 yuqi1129 Aug 26, 2024

Choose a reason for hiding this comment

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

Only MySQL use MysqlConstants.GRAVITINO_AUTO_INCREMENT_OFFSET_KEY and supports the feature auto_increment now.

@jerryshao jerryshao merged commit 0af3353 into apache:main Aug 26, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Shrink JDBC catalog binary package size.
3 participants