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

[#3514] improvement(flink-connector): add flink-connector-runtime to build flink connector #4791

Merged
merged 12 commits into from
Aug 30, 2024

Conversation

coolderli
Copy link
Contributor

What changes were proposed in this pull request?

  • add flink connector

Why are the changes needed?

Fix: #3514

Does this PR introduce any user-facing change?

  • no

How was this patch tested?

  • local test

@coolderli coolderli force-pushed the add-flink-connector-runtime branch from 3017555 to d591c73 Compare August 29, 2024 16:10
@coolderli coolderli changed the title [#3514][flink-connector]: add flink-connector-runtime to build flink connector [#3514] improvement(flink-connector): add flink-connector-runtime to build flink connector Aug 29, 2024
@coolderli
Copy link
Contributor Author

@FANNG1 PTAL. Thanks.

@jerryshao
Copy link
Contributor

Hi @coolderli without this runtime jar, how do we run Flink Gravitino connector previously?

@coolderli
Copy link
Contributor Author

Hi @coolderli without this runtime jar, how do we run Flink Gravitino connector previously?

@jerryshao Just use the flink connector jar. But there are some conflicts here. Therefore, a shaded operation was performed in the runtime.

@@ -23,7 +23,7 @@ This capability allows users to perform federation queries, accessing data from

## How to use it

1. [Build](../how-to-build.md) or [download](https://mvnrepository.com/artifact/org.apache.gravitino/gravitino-flink-connector) the Gravitino flink connector jar, and place it to the classpath of Flink.
1. [Build](../how-to-build.md) or [download](https://mvnrepository.com/artifact/org.apache.gravitino/gravitino-flink-connector) the Gravitino flink connector runtime jar, and place it to the classpath of Flink.
Copy link
Contributor

Choose a reason for hiding this comment

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

@yuqi please help to verify that the flink runtime jar link will be https://mvnrepository.com/artifact/org.apache.gravitino/gravitino-flink-connector-runtime ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me check it.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no artifact named gravitino-flink-connector-runtime, only gravitino-flink-connector-runtime-1.18.0_2.12 and gravitino-flink_2.12.

Copy link
Contributor

Choose a reason for hiding this comment

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

so the download link should be https://mvnrepository.com/artifact/org.apache.gravitino/gravitino-flink-connector-runtime-1.18.0_2.12?

Copy link
Contributor

Choose a reason for hiding this comment

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

we should use 1.18 not 1.18.0 here? and link should be https://mvnrepository.com/artifact/org.apache.gravitino/gravitino-flink-connector-runtime-1.18 like spark not include scala versions.

val flinkMajorVersion: String = flink.substringBeforeLast(".")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@FANNG1 But I saw the spark has the scala version(_2.12).
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean the runtime jar download link doesn't contain scala version

Copy link
Contributor

Choose a reason for hiding this comment

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

settings.gradle.kts Outdated Show resolved Hide resolved
@FANNG1
Copy link
Contributor

FANNG1 commented Aug 30, 2024

LGTM, except minor comments, @coolderli could you check it again?

settings.gradle.kts Outdated Show resolved Hide resolved
FANNG1
FANNG1 previously approved these changes Aug 30, 2024
@FANNG1
Copy link
Contributor

FANNG1 commented Aug 30, 2024

LGTM

Comment on lines +42 to +45
implementation(project(":api"))
implementation(project(":catalogs:catalog-common"))
implementation(project(":common"))
implementation(project(":core"))
Copy link
Contributor

Choose a reason for hiding this comment

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

you can add { exclude("*") } to shrink the size of lib

Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused why you need to depend on core module?

Copy link
Contributor

@FANNG1 FANNG1 Aug 30, 2024

Choose a reason for hiding this comment

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

@mchades could you create a separate issue to track 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.

LMTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

@mchades could you create a separate issue to track this?

If this is just a minor fix, it is recommended to fix it in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid there maybe something unexpected, I will merge this PR to kick off 0.6.0

@FANNG1 FANNG1 merged commit 6e4e103 into apache:main Aug 30, 2024
27 checks passed
github-actions bot pushed a commit that referenced this pull request Aug 30, 2024
…build flink connector (#4791)

### What changes were proposed in this pull request?

- add flink runtime connector

### Why are the changes needed?

Fix: #3514 

### Does this PR introduce _any_ user-facing change?

- no

### How was this patch tested?

- local test

---------

Co-authored-by: fanng <[email protected]>
@FANNG1
Copy link
Contributor

FANNG1 commented Aug 30, 2024

@coolderli , thanks for your work!

FANNG1 pushed a commit that referenced this pull request Oct 15, 2024
…ime lib size from 84MB to 16MB (#4819)

### What changes were proposed in this pull request?

- shrink the flink-connector-runtime jar.
- Change client-java scope from implementation to CompileOnly in flink
module.
- use the client-java-runtime in flink-connector-runtime.

### Why are the changes needed?
Fix: #5145 


- The flink-connector-runtime jar is much big as mentioned in
#4791 (comment).


### Does this PR introduce _any_ user-facing change?

- no

### How was this patch tested?

- local test
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.

[Subtask] [flink-connector] add flink-connector-runtime to build Flink connector
5 participants