Hisoka-X commented on code in PR #5985:
URL: https://github.com/apache/seatunnel/pull/5985#discussion_r1421984203
##########
seatunnel-translation/seatunnel-translation-spark/seatunnel-translation-spark-common/src/main/java/org/apache/seatunnel/translation/spark/serialization/InternalRowCollector.java:
##########
@@ -50,7 +48,7 @@ public InternalRowCollector(
this.rowSerialization = new InternalRowConverter(dataType);
this.collectTotalCount = new AtomicLong(0);
this.envOptions = (Map) envOptionsInfo;
- FlowControlStrategy flowControlStrategy =
getFlowControlStrategy(envOptions);
+ FlowControlStrategy flowControlStrategy =
FlowControlStrategy.fromMap(envOptions);
Review Comment:
> why design this can be null
null means not enabling this feature, so we can save unnecessary method
calls.
> and setting default value (Integer.MAX_VALUE) in flow control strategy
default value as `Integer.MAX_VALUE` only worked on only one of the number
of rows or bytes set by the user will take effect. In fact, `Integer.MAX_VALUE`
basically represents unlimited. eg: user config bytes == 10, so we will set
rows count as `Integer.MAX_VALUE`. If we feel there is something wrong with the
current code, we can improve it.
> In the implementation of the three engines, this is completely
inconsistent.
Which part？
> Where is the detailed STIP for this feature?
I personally feel that the amount of changes to this function is very small,
and it is also a small function with little impact. Not all feature require
STIP. If you think this feature requires a STIP, please let me know when
created the PR.
##########
seatunnel-translation/seatunnel-translation-spark/seatunnel-translation-spark-common/src/main/java/org/apache/seatunnel/translation/spark/serialization/InternalRowCollector.java:
##########
@@ -50,7 +48,7 @@ public InternalRowCollector(
this.rowSerialization = new InternalRowConverter(dataType);
this.collectTotalCount = new AtomicLong(0);
this.envOptions = (Map) envOptionsInfo;
- FlowControlStrategy flowControlStrategy =
getFlowControlStrategy(envOptions);
+ FlowControlStrategy flowControlStrategy =
FlowControlStrategy.fromMap(envOptions);
Review Comment:
> why design this can be null
null means not enabling this feature, so we can save unnecessary method
calls.
> and setting default value (Integer.MAX_VALUE) in flow control strategy
default value as `Integer.MAX_VALUE` only worked on only one of the number
of rows or bytes set by the user will take effect. In fact, `Integer.MAX_VALUE`
basically represents unlimited. eg: user config bytes == 10, so we will set
rows count as `Integer.MAX_VALUE`. If we feel there is something wrong with the
current code, we can improve it.
> In the implementation of the three engines, this is completely
inconsistent.
Which part？
> Where is the detailed STIP for this feature?
I personally feel that the amount of changes to this function is very small,
and it is also a small function with little impact. Not all feature require
STIP. If you think this feature requires a STIP, please let me know when
created the PR.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
