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

[Feature][Connectors-v2-file-ftp] FTP source/sink add ftp connection mode (#6077) #6084

Closed
wants to merge 5 commits into from

Conversation

WilliamTan778
Copy link
Contributor

[Fix] [Connectors-v2-file-ftp] FTP source|sink add ftp connection mode (#6077)

Purpose of this pull request

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

@CheneyYin
Copy link
Contributor

Maybe we also need to add test cases to verify that the new parameter active_mode_status works.

@Hisoka-X
Copy link
Member

Maybe we also need to add test cases to verify that the new parameter active_mode_status works.

Yes. Please add test case for this @WilliamTan778 .Thanks

@WilliamTan778
Copy link
Contributor Author

@CheneyYin @Hisoka-X receive , thanks

@Hisoka-X Hisoka-X added the feature New feature label Dec 26, 2023
@Hisoka-X Hisoka-X changed the title [Fix] [Connectors-v2-file-ftp] FTP source|sink add ftp connection mode (#6077) [Feature] [Connectors-v2-file-ftp] FTP source/sink add ftp connection mode (#6077) Dec 26, 2023
@Hisoka-X Hisoka-X changed the title [Feature] [Connectors-v2-file-ftp] FTP source/sink add ftp connection mode (#6077) [Feature][Connectors-v2-file-ftp] FTP source/sink add ftp connection mode (#6077) Dec 26, 2023
@WilliamTan778
Copy link
Contributor Author

WilliamTan778 commented Dec 27, 2023

@CheneyYin @Hisoka-X This issue requires the deployment of containers that enable NAT. Currently, it is difficult to configure the operation of enabling NAT(Network Address Translation) in FTP containers. After testing several configurations, it is maybe not possible, so only edit config set active_mode_status=true use FTP active mode , No passive mode testing provided , thanks

Comment on lines 39 to 43
public static final Option<Boolean> FTP_ACTIVE_MODE_STATUS =
Options.key("active_mode_status")
.booleanType()
.defaultValue(true)
.withDescription("FTP server mode default active");
Copy link
Member

Choose a reason for hiding this comment

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

I think we should use enum to support 4 type mode.
image
Not only true or false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am making the necessary modifications , thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://commons.apache.org/proper/commons-net/apidocs/org/apache/commons/net/ftp/FTPClient.html
image

According to the above content, it can be seen that the Remote mode is server to server (FXP), while our product is in client and server mode, so currently only active is support active_local 、passive_local , thank you very much

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have created a new pull request and merged the commit. Thank you very much

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@WilliamTan778 WilliamTan778 deleted the ftp_change branch December 28, 2023 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants