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

[env] Implement all methods of env_flink #13

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

masteryhx
Copy link
Collaborator

@masteryhx masteryhx commented Mar 15, 2024

What is the purpose of the change

Implemet all methods of env_flink

Brief change log

  • re-construct jni_helper
  • Implement all methods of env_flink

Verifying this change

This change will be tested in Java side after FlinkEnv is ready.

Copy link
Collaborator

@ljz2051 ljz2051 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I left some comments. PTAL

env/flink/env_flink.cc Outdated Show resolved Hide resolved
env/flink/env_flink.cc Outdated Show resolved Hide resolved
env/flink/env_flink.cc Show resolved Hide resolved
env/flink/env_flink.cc Show resolved Hide resolved
env/flink/env_flink.cc Outdated Show resolved Hide resolved
env/flink/env_flink.cc Outdated Show resolved Hide resolved
env/flink/env_flink.cc Outdated Show resolved Hide resolved
env/flink/env_flink.cc Outdated Show resolved Hide resolved
env/flink/env_flink.cc Outdated Show resolved Hide resolved
@ljz2051 ljz2051 requested a review from curcur March 19, 2024 07:48
@masteryhx masteryhx force-pushed the implment_env_flink branch 2 times, most recently from 3cf7cf9 to 684cecb Compare March 19, 2024 08:35
@masteryhx masteryhx requested a review from ljz2051 March 19, 2024 08:39
Copy link
Collaborator

@ljz2051 ljz2051 left a comment

Choose a reason for hiding this comment

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

Thanks for the update. PTAL my comments.

env/flink/env_flink.h Outdated Show resolved Hide resolved
env/flink/env_flink.h Outdated Show resolved Hide resolved
env/flink/jni_helper.cc Show resolved Hide resolved
env/flink/jni_helper.cc Show resolved Hide resolved
env/flink/env_flink.cc Show resolved Hide resolved
env/flink/env_flink.cc Outdated Show resolved Hide resolved
@masteryhx masteryhx requested a review from ljz2051 March 20, 2024 10:09
Copy link
Collaborator

@ljz2051 ljz2051 left a comment

Choose a reason for hiding this comment

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

Overall, LGTM.
As we discussed offline, we should add necessary UT tests for these filesystem interface later.

@masteryhx masteryhx merged commit a5c920d into ververica:main Mar 21, 2024
10 checks passed
@masteryhx masteryhx deleted the implment_env_flink branch March 21, 2024 04:52
fredia pushed a commit to fredia/ForSt that referenced this pull request Sep 26, 2024
fredia pushed a commit to fredia/ForSt that referenced this pull request Sep 26, 2024
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.

2 participants