You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@seatunnel.apache.org by GitBox <gi...@apache.org> on 2022/02/12 09:19:48 UTC
[GitHub] [incubator-seatunnel] yx91490 opened a new pull request #1236: [SeaTunnel #1092] [core] Fix "Can only specify option -i once" in command line args parser
yx91490 opened a new pull request #1236:
URL: https://github.com/apache/incubator-seatunnel/pull/1236
<!--
Thank you for contributing to SeaTunnel! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
## Contribution Checklist
- Make sure that the pull request corresponds to a [GITHUB issue](https://github.com/apache/incubator-seatunnel/issues).
- Name the pull request in the form "[Feature] [component] Title of the pull request", where *Feature* can be replaced by `Hotfix`, `Bug`, etc.
- Minor fixes should be named following this pattern: `[hotfix] [docs] Fix typo in README.md doc`.
-->
## Purpose of this pull request
close #1229
<!-- Describe the purpose of this pull request. For example: This pull request adds checkstyle plugin.-->
## Check list
* [ ] Code changed are covered with tests, or it does not need tests for reason:
* [ ] If any new Jar binary package adding in you PR, please add License Notice according
[New License Guide](https://github.com/apache/incubator-seatunnel/blob/dev/docs/en/developement/NewLicenseGuide.md)
* [ ] If necessary, please update the documentation to describe the new feature. https://github.com/apache/incubator-seatunnel/tree/dev/docs
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] CalvinKirs merged pull request #1236: [Bug] [config] Fix "Can only specify option -i once" in command line args parser
Posted by GitBox <gi...@apache.org>.
CalvinKirs merged pull request #1236:
URL: https://github.com/apache/incubator-seatunnel/pull/1236
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] yx91490 commented on pull request #1236: [Bug] [config] Fix "Can only specify option -i once" in command line args parser
Posted by GitBox <gi...@apache.org>.
yx91490 commented on pull request #1236:
URL: https://github.com/apache/incubator-seatunnel/pull/1236#issuecomment-1040264957
> Hi @yx91490, we nerver use `variables` in the main logic.
I think this quietly a bug, isn't it?
this pr is the smallest code modification to fix this bug, which is the same logic as original.
If you think the original code's logical had problems, maybe we can discuss in another issue.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] ruanwenjun commented on pull request #1236: [Bug] [config] Fix "Can only specify option -i once" in command line args parser
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on pull request #1236:
URL: https://github.com/apache/incubator-seatunnel/pull/1236#issuecomment-1040320879
> > Hi @yx91490, we nerver use `variables` in the main logic.
>
> I think this quietly a bug, isn't it? this pr is the smallest code modification to fix this bug, which is the same logic as original. If you think the original code's logical had problems, maybe we can discuss in another issue.
As @leo65535 said, the `variables` is never used, so it might be better to remove it.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] leo65535 commented on pull request #1236: [Bug] [config] Fix "Can only specify option -i once" in command line args parser
Posted by GitBox <gi...@apache.org>.
leo65535 commented on pull request #1236:
URL: https://github.com/apache/incubator-seatunnel/pull/1236#issuecomment-1040000287
Hi @yx91490, we nerver use `variables` in the main logic.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] yx91490 commented on pull request #1236: [Bug] [config] Fix "Can only specify option -i once" in command line args parser
Posted by GitBox <gi...@apache.org>.
yx91490 commented on pull request #1236:
URL: https://github.com/apache/incubator-seatunnel/pull/1236#issuecomment-1041584786
> > > Hi @yx91490, we nerver use `variables` in the main logic.
> >
> >
> > I think this quietly a bug, isn't it? this pr is the smallest code modification to fix this bug, which is the same logic as original. If you think the original code's logical had problems, maybe we can discuss in another issue.
>
> As @leo65535 said, the `variables` is never used, so it might be better to remove it.
remove it is not a easy and the best way to fix this, because in start-seatunnel-spark.sh script, all arguments will be passed to java code, and finally be parsed by jcommander, but currently jcommander will throw exception if input an unknown parameter. whether modify start script or jcomander will pay much effort to ensure compatibility and not to introduce new bugs, which is poles apart with the issue's original intention: just fix an obviously bug that was introduced by a unequal code translation.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] ruanwenjun commented on pull request #1236: [Bug] [config] Fix "Can only specify option -i once" in command line args parser
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on pull request #1236:
URL: https://github.com/apache/incubator-seatunnel/pull/1236#issuecomment-1040320879
> > Hi @yx91490, we nerver use `variables` in the main logic.
>
> I think this quietly a bug, isn't it? this pr is the smallest code modification to fix this bug, which is the same logic as original. If you think the original code's logical had problems, maybe we can discuss in another issue.
As @leo65535 said, the `variables` is never used, so it might be better to remove it.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] leo65535 commented on pull request #1236: [Bug] [config] Fix "Can only specify option -i once" in command line args parser
Posted by GitBox <gi...@apache.org>.
leo65535 commented on pull request #1236:
URL: https://github.com/apache/incubator-seatunnel/pull/1236#issuecomment-1040000287
Hi @yx91490, we nerver use `variables` in the main logic.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] yx91490 commented on pull request #1236: [Bug] [config] Fix "Can only specify option -i once" in command line args parser
Posted by GitBox <gi...@apache.org>.
yx91490 commented on pull request #1236:
URL: https://github.com/apache/incubator-seatunnel/pull/1236#issuecomment-1040264957
> Hi @yx91490, we nerver use `variables` in the main logic.
I think this quietly a bug, isn't it?
this pr is the smallest code modification to fix this bug, which is the same logic as original.
If you think the original code's logical had problems, maybe we can discuss in another issue.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] yx91490 commented on a change in pull request #1236: [Bug] [config] Fix "Can only specify option -i once" in command line args parser
Posted by GitBox <gi...@apache.org>.
yx91490 commented on a change in pull request #1236:
URL: https://github.com/apache/incubator-seatunnel/pull/1236#discussion_r816787650
##########
File path: seatunnel-core/seatunnel-core-base/src/test/java/org/apache/seatunnel/config/command/CommandSparkArgsTest.java
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.seatunnel.config.command;
+
+import com.beust.jcommander.JCommander;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Collections;
+
+public class CommandSparkArgsTest {
+
+ @Test
+ public void testParseSparkArgs() {
+ String[] args = {"-c", "app.conf", "-e", "client", "-m", "yarn", "-i", "city=shijiazhuang"};
Review comment:
have update, PTAL, thanks
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] ruanwenjun commented on a change in pull request #1236: [Bug] [config] Fix "Can only specify option -i once" in command line args parser
Posted by GitBox <gi...@apache.org>.
ruanwenjun commented on a change in pull request #1236:
URL: https://github.com/apache/incubator-seatunnel/pull/1236#discussion_r816484136
##########
File path: seatunnel-core/seatunnel-core-base/src/test/java/org/apache/seatunnel/config/command/CommandSparkArgsTest.java
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.seatunnel.config.command;
+
+import com.beust.jcommander.JCommander;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Collections;
+
+public class CommandSparkArgsTest {
+
+ @Test
+ public void testParseSparkArgs() {
+ String[] args = {"-c", "app.conf", "-e", "client", "-m", "yarn", "-i", "city=shijiazhuang"};
Review comment:
```suggestion
String[] args = {"-c", "app.conf", "-e", "client", "-m", "yarn", "-i", "city=shijiazhuang", "-i", "name=Tom"};
```
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] yx91490 edited a comment on pull request #1236: [Bug] [config] Fix "Can only specify option -i once" in command line args parser
Posted by GitBox <gi...@apache.org>.
yx91490 edited a comment on pull request #1236:
URL: https://github.com/apache/incubator-seatunnel/pull/1236#issuecomment-1041584786
> > > Hi @yx91490, we nerver use `variables` in the main logic.
> >
> >
> > I think this quietly a bug, isn't it? this pr is the smallest code modification to fix this bug, which is the same logic as original. If you think the original code's logical had problems, maybe we can discuss in another issue.
>
> As @leo65535 said, the `variables` is never used, so it might be better to remove it.
remove it is not a easy and the best way to fix this, because in start-seatunnel-spark.sh script, all arguments will be passed to java code, and finally be parsed by jcommander, but currently jcommander will throw exception if input an unknown parameter. whether modify start script or jcomander will pay much effort to ensure compatibility and not to introduce new bugs, which is poles apart with the issue's original intention: just fix an obviously bug that was introduced by a unequal code translation.
Hope you can understand, thanks.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-seatunnel] leo65535 commented on pull request #1236: [Bug] [config] Fix "Can only specify option -i once" in command line args parser
Posted by GitBox <gi...@apache.org>.
leo65535 commented on pull request #1236:
URL: https://github.com/apache/incubator-seatunnel/pull/1236#issuecomment-1042814550
@yx91490 thanks for your detail description. Just from my side, I think it is not necessary to modify these changes.
--
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: commits-unsubscribe@seatunnel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org