luoyuxia commented on code in PR #22831:
URL: https://github.com/apache/flink/pull/22831#discussion_r1241007352


##########
flink-connectors/flink-connector-files/src/main/java/org/apache/flink/connector/file/table/PartitionCommitPolicyFactory.java:
##########
@@ -37,12 +39,22 @@ public class PartitionCommitPolicyFactory implements 
Serializable {
     private final String policyKind;
     private final String customClass;
     private final String successFileName;
+    private final List<String> parameters;
 
     public PartitionCommitPolicyFactory(

Review Comment:
   We can remove this constructor. 
   Please also rememer to update the Hive part.



##########
flink-connectors/flink-connector-files/src/main/java/org/apache/flink/connector/file/table/FileSystemConnectorOptions.java:
##########
@@ -220,6 +221,15 @@ public class FileSystemConnectorOptions {
                             "The partition commit policy class for implement"
                                     + " PartitionCommitPolicy interface. Only 
work in custom commit policy");
 
+    public static final ConfigOption<List<String>> 
SINK_PARTITION_COMMIT_POLICY_CLASS_PARAMETERS =
+            key("sink.partition-commit.policy.class.parameters")
+                    .stringType()
+                    .asList()
+                    .noDefaultValue()
+                    .withDescription(
+                            "A (semicolon-separated) list of patterns that for 
custom policy class"

Review Comment:
   May be we should highlight that the all the parameter(if have) of this 
policy class should be string.



##########
flink-connectors/flink-connector-files/src/main/java/org/apache/flink/connector/file/table/PartitionCommitPolicyFactory.java:
##########
@@ -63,11 +75,23 @@ public List<PartitionCommitPolicy> createPolicyChain(
                                             successFileName, fsSupplier.get());
                                 case PartitionCommitPolicy.CUSTOM:
                                     try {
-                                        return (PartitionCommitPolicy)
-                                                
cl.loadClass(customClass).newInstance();
-                                    } catch (ClassNotFoundException
-                                            | IllegalAccessException
-                                            | InstantiationException e) {
+                                        if (parameters != null && 
!parameters.isEmpty()) {

Review Comment:
   I'm wondering if change it to 
   ```
   parameters != null
   ```
   Can it also get the constructor with empty arguments?
   If so, we should consider to change to `parameters != null` to make code 
more clear.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to