fsk119 commented on a change in pull request #15332:
URL: https://github.com/apache/flink/pull/15332#discussion_r600097296
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliClient.java
##########
@@ -424,28 +424,14 @@ private void callReset(SqlCommandCall cmdCall) {
else {
String key = cmdCall.operands[0].trim();
executor.resetSessionProperty(sessionId, key);
- if (YamlConfigUtils.isRemovedKey(key)) {
- terminal.writer()
-
.println(CliStrings.messageWarning(MESSAGE_SET_REMOVED_KEY).toAnsi());
- } else {
- if (YamlConfigUtils.isDeprecatedKey(key)) {
- terminal.writer()
- .println(
- CliStrings.messageWarning(
- String.format(
-
MESSAGE_SET_DEPRECATED_KEY,
- key,
- YamlConfigUtils
-
.getOptionNameWithDeprecatedKey(
-
key)))
- .toAnsi());
- }
+ printRemovedAndDeprecatedKeyMessage(key);
+ if (!YamlConfigUtils.isRemovedKey(key)) {
Review comment:
The semantic is a little weird. I think the logic should work like
```
if (isRemovedKey || isDeprecatedKey) {
printRemovedAndDeprecatedKeyMessage(key);
} else {
...
}
```
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/cli/CliClient.java
##########
@@ -481,26 +468,12 @@ private void callSet(SqlCommandCall cmdCall) {
printExecutionException(e);
return;
}
- if (YamlConfigUtils.isRemovedKey(key)) {
- terminal.writer()
-
.println(CliStrings.messageWarning(MESSAGE_SET_REMOVED_KEY).toAnsi());
- } else {
- if (YamlConfigUtils.isDeprecatedKey(key)) {
- terminal.writer()
- .println(
- CliStrings.messageWarning(
- String.format(
-
MESSAGE_SET_DEPRECATED_KEY,
- key,
- YamlConfigUtils
-
.getOptionNameWithDeprecatedKey(
-
key)))
- .toAnsi());
- }
+ printRemovedAndDeprecatedKeyMessage(key);
+ if (!YamlConfigUtils.isRemovedKey(key)) {
Review comment:
ditto
##########
File path:
flink-table/flink-sql-client/src/main/java/org/apache/flink/table/client/config/YamlConfigUtils.java
##########
@@ -170,7 +170,7 @@ public static String getOptionNameWithDeprecatedKey(String
key) {
return ENTRY_TO_OPTION.get(key);
}
- public static boolean isOptionKey(String key) {
+ public static boolean isOptionContainDeprecatedKey(String key) {
Review comment:
What about `isOptionHasDeprecatedKey`?
##########
File path: flink-table/flink-sql-client/src/test/resources/sql/set.q
##########
@@ -104,3 +104,70 @@ Was expecting one of:
"," ...
!error
+
+# test reset remove key
+reset execution.max-idle-state-retention;
+[WARNING] The specified key is not supported anymore.
+!warning
+
+set execution.max-table-result-rows=100;
+[WARNING] The specified key 'execution.max-table-result-rows' is deprecated.
Please use 'sql-client.execution.max-table-result.rows' instead.
+[INFO] Session property has been set.
+!warning
+
+# test reset the deprecated key
+reset execution.max-table-result-rows;
+[WARNING] The specified key 'execution.max-table-result-rows' is deprecated.
Please use 'sql-client.execution.max-table-result.rows' instead.
+[INFO] Session property has been reset.
+!warning
+
+set;
+execution.attached=true
+execution.savepoint.ignore-unclaimed-state=false
+execution.shutdown-on-attached-exit=false
+execution.target=remote
+jobmanager.rpc.address=$VAR_JOBMANAGER_RPC_ADDRESS
+pipeline.classpaths=
+pipeline.jars=$VAR_PIPELINE_JARS
+rest.port=$VAR_REST_PORT
+!ok
+
+
+set parallelism.default=3;
+[INFO] Session property has been set.
+!info
+
+# test reset option key
+reset parallelism.default;
Review comment:
emmm. My suggestion is to set option but reset deprecated key. But this
case is to set option but reset option.
But I think the case above is enough to resolve the comments.
```
set execution.max-table-result-rows=200;
reset sql-client.execution.max-table-result.rows;
```
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
