-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[improve] update localfile connector config #8765
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
[improve] update localfile connector config #8765
Conversation
...rg/apache/seatunnel/connectors/seatunnel/file/local/source/config/LocalFileSourceConfig.java
Outdated
Show resolved
Hide resolved
...base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/config/BaseSinkConfig.java
Outdated
Show resolved
Hide resolved
# Conflicts: # seatunnel-connectors-v2/connector-file/connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/config/BaseFileSinkConfig.java # seatunnel-connectors-v2/connector-file/connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/config/FileBaseSinkOptions.java # seatunnel-connectors-v2/connector-file/connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/source/reader/AbstractReadStrategy.java # seatunnel-connectors-v2/connector-file/connector-file-cos/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/cos/sink/CosFileSinkFactory.java # seatunnel-connectors-v2/connector-file/connector-file-cos/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/cos/source/CosFileSourceFactory.java # seatunnel-connectors-v2/connector-file/connector-file-ftp/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/ftp/sink/FtpFileSinkFactory.java # seatunnel-connectors-v2/connector-file/connector-file-ftp/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/ftp/source/FtpFileSourceFactory.java # seatunnel-connectors-v2/connector-file/connector-file-hadoop/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/hdfs/sink/HdfsFileSinkFactory.java # seatunnel-connectors-v2/connector-file/connector-file-hadoop/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/hdfs/source/HdfsFileSourceFactory.java # seatunnel-connectors-v2/connector-file/connector-file-jindo-oss/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/oss/jindo/sink/OssFileSinkFactory.java # seatunnel-connectors-v2/connector-file/connector-file-jindo-oss/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/oss/jindo/source/OssFileSourceFactory.java # seatunnel-connectors-v2/connector-file/connector-file-local/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/local/sink/LocalFileSinkFactory.java # seatunnel-connectors-v2/connector-file/connector-file-local/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/local/source/LocalFileSourceFactory.java # seatunnel-connectors-v2/connector-file/connector-file-obs/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/obs/sink/ObsFileSinkFactory.java # seatunnel-connectors-v2/connector-file/connector-file-obs/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/obs/source/ObsFileSourceFactory.java # seatunnel-connectors-v2/connector-file/connector-file-oss/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/oss/source/OssFileSourceFactory.java # seatunnel-connectors-v2/connector-file/connector-file-s3/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/s3/source/S3FileSourceFactory.java # seatunnel-connectors-v2/connector-file/connector-file-sftp/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/sftp/sink/SftpFileSinkFactory.java # seatunnel-connectors-v2/connector-file/connector-file-sftp/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/sftp/source/SftpFileSourceFactory.java
seatunnel-ci-tools/src/test/java/org/apache/seatunnel/api/ConnectorOptionCheckTest.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/seatunnel/connectors/seatunnel/file/cos/config/CosConfigOptions.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thanks @misi1987107
...n/java/org/apache/seatunnel/connectors/seatunnel/file/sftp/source/SftpFileSourceFactory.java
Show resolved
Hide resolved
.../main/java/org/apache/seatunnel/connectors/seatunnel/file/sftp/sink/SftpFileSinkFactory.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @misi1987107 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request updates the localfile connector configuration by renaming and refactoring configuration option classes from Base* to FileBase* and synchronizing key references across source, sink, and test files. Key changes include:
- Renaming configuration classes and updating option key references.
- Adjustments to option descriptions and default values in connector source and sink implementations.
- Updates to test cases and read/write strategies to use the new FileBase* option classes.
Reviewed Changes
Copilot reviewed 98 out of 98 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/config/FileBaseSourceOptions.java | Added several new option definitions and updated descriptions. |
connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/config/FileBaseOptions.java | Renamed from BaseSourceConfigOptions and updated option descriptions. |
connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/config/FileBaseSinkOptions.java | Renamed and updated option key references in sink config. |
Various source/reader/sink files | Updated to use FileBase* options instead of Base* options. |
ConnectorOptionCheckTest.java | Added additional type checks to support the new option classes. |
Comments suppressed due to low confidence (2)
connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/config/FileBaseOptions.java:40
- The description for the file path option was changed from 'source files' to 'target files', which may be inconsistent with the connector's intended usage.
.withDescription("The file path of target files");
connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/source/reader/CsvReadStrategy.java:190
- The hard-coded assignment of fieldDelimiter to ',' overwrites any configured value from the plugin configuration, which could lead to unexpected behavior.
fieldDelimiter = ",";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 if CI passed
.optional(BaseSinkConfig.SCHEMA_SAVE_MODE) | ||
.optional(BaseSinkConfig.DATA_SAVE_MODE) | ||
.optional(FileBaseSinkOptions.TMP_PATH) | ||
.optional(FileBaseSinkOptions.FILE_FORMAT_TYPE) | ||
.optional(FileBaseSinkOptions.SCHEMA_SAVE_MODE) | ||
.optional(FileBaseSinkOptions.DATA_SAVE_MODE) | ||
.conditional( | ||
BaseSinkConfig.FILE_FORMAT_TYPE, | ||
FileFormat.TEXT, | ||
BaseSinkConfig.ROW_DELIMITER, | ||
BaseSinkConfig.FIELD_DELIMITER, | ||
BaseSinkConfig.TXT_COMPRESS, | ||
BaseSinkConfig.ENABLE_HEADER_WRITE) | ||
.conditional( | ||
BaseSinkConfig.FILE_FORMAT_TYPE, | ||
FileFormat.CSV, | ||
BaseSinkConfig.TXT_COMPRESS, | ||
BaseSinkConfig.ENABLE_HEADER_WRITE) | ||
.conditional( | ||
BaseSinkConfig.FILE_FORMAT_TYPE, | ||
FileFormat.JSON, | ||
BaseSinkConfig.TXT_COMPRESS) | ||
.conditional( | ||
BaseSinkConfig.FILE_FORMAT_TYPE, | ||
FileFormat.ORC, | ||
BaseSinkConfig.ORC_COMPRESS) | ||
.conditional( | ||
BaseSinkConfig.FILE_FORMAT_TYPE, | ||
FileFormat.PARQUET, | ||
BaseSinkConfig.PARQUET_COMPRESS, | ||
BaseSinkConfig.PARQUET_AVRO_WRITE_FIXED_AS_INT96, | ||
BaseSinkConfig.PARQUET_AVRO_WRITE_TIMESTAMP_AS_INT96) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found so many conditional be deleted. Why should do that? @misi1987107 . cc @davidzollo @liunaijie
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted by #9018
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the
conditional( @nonnull Option conditionalOption,
@nonnull T expectValue,
@nonnull Option<?>... requiredOptions)
method is a required parameter that meets the specified conditions, but these are not required options,
So I deleted these.
Such as FILE_FORMAT_TYPE is Text,but TXT_COMPRESS is not a required parameter.
Did I misunderstand,give me some pointer?
Purpose of this pull request
subtask of #8576
Does this PR introduce any user-facing change?
How was this patch tested?
Check list
New License Guide
release-note
.