You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by "chrisdutz (via GitHub)" <gi...@apache.org> on 2024/01/11 10:27:57 UTC
[PR] Feat/cli with overridable streams [iotdb]
chrisdutz opened a new pull request, #11885:
URL: https://github.com/apache/iotdb/pull/11885
In order to provide a WebCLI, I needed to be able to pass in InputStream and OutputStreams to the CLI, so I could redirect input and output to a WebSocket.
I updated the CLI so when using the "main" it simply initializes the CliContext with System.stdin and System.stdout, but also allows calling a static runCli method where I can pass in a manually constructed CliContext.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feat/cli with overridable streams [iotdb]
Posted by "CritasWang (via GitHub)" <gi...@apache.org>.
CritasWang commented on PR #11885:
URL: https://github.com/apache/iotdb/pull/11885#issuecomment-1903217346
hi @chrisdutz
This change will cause the CLI to not work properly.
For example:
The input statement will be repeatedly output
Unable to auto-complete keywords
Unable to select SQL statement by up and down keys
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feat/cli with overridable streams [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on code in PR #11885:
URL: https://github.com/apache/iotdb/pull/11885#discussion_r1468817074
##########
iotdb-client/cli/src/main/java/org/apache/iotdb/cli/IoTDBSyntaxHighlighter.java:
##########
@@ -67,12 +67,14 @@ public AttributedString highlight(LineReader reader, String buffer) {
@Override
public void setErrorPattern(Pattern errorPattern) {
- IoTPrinter.print("");
+ // TODO: Not quite sure what this code should actually do
Review Comment:
Yeah ... I added comments to code that I didn't understand ... in this particular case I couldn't see what the code actually did, so I added a comment.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feat/cli with overridable streams [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz merged PR #11885:
URL: https://github.com/apache/iotdb/pull/11885
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feat/cli with overridable streams [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on PR #11885:
URL: https://github.com/apache/iotdb/pull/11885#issuecomment-1900220519
However I think there might be one singleton that I learned too late that it is needed ...
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feat/cli with overridable streams [iotdb]
Posted by "CritasWang (via GitHub)" <gi...@apache.org>.
CritasWang commented on PR #11885:
URL: https://github.com/apache/iotdb/pull/11885#issuecomment-1888369697
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feat/cli with overridable streams [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on PR #11885:
URL: https://github.com/apache/iotdb/pull/11885#issuecomment-1905954873
This is really odd ... as it shouldn't change anything, just instead of using "System.in" and "System.out" you use variables that are initialized with them.
But I'll have a look, if I can spot what's causing this.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feat/cli with overridable streams [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on PR #11885:
URL: https://github.com/apache/iotdb/pull/11885#issuecomment-1906482454
Ok ... was a bit tricky, but in the end I pushed a change that should fix it. There were actually two issues:
1. I was running the CLI in IntelliJ (not from the console) ... here both version behaved identically (Even when running the CLI from master in IntelliJ directly, it will echo the commands and not have auto-complete).
2. I was expecting to setting "in" and "out" to System.in and System.out woudln't be any change, but it turns out in this case the internal variables "in" and "out" are not null and the code will take a wrong turn.
I updated the code to simply not set the "streams" if "in == System.in && out == System.out" ... that seemed to have worked.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feat/cli with overridable streams [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on PR #11885:
URL: https://github.com/apache/iotdb/pull/11885#issuecomment-1891853389
Thanks for spotting this ... issue now seems to be solved.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feat/cli with overridable streams [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on PR #11885:
URL: https://github.com/apache/iotdb/pull/11885#issuecomment-1900219568
There is one for just the static stuff ... Just for my current work, I need both changes, so I based this PR off the other one.
https://github.com/apache/iotdb/pull/11870
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feat/cli with overridable streams [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on PR #11885:
URL: https://github.com/apache/iotdb/pull/11885#issuecomment-1906035361
But I can confirm, that the code seems to be behaving differently, even if it shouldn't be. I just switched to master, rebuilt and ran the CLI and can see what you were referring to. I'll try to figure out what's causing the difference.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feat/cli with overridable streams [iotdb]
Posted by "OneSizeFitsQuorum (via GitHub)" <gi...@apache.org>.
OneSizeFitsQuorum commented on code in PR #11885:
URL: https://github.com/apache/iotdb/pull/11885#discussion_r1468811565
##########
iotdb-client/cli/src/main/java/org/apache/iotdb/cli/IoTDBSyntaxHighlighter.java:
##########
@@ -67,12 +67,14 @@ public AttributedString highlight(LineReader reader, String buffer) {
@Override
public void setErrorPattern(Pattern errorPattern) {
- IoTPrinter.print("");
+ // TODO: Not quite sure what this code should actually do
+ new IoTPrinter(System.out).print("");
}
@Override
public void setErrorIndex(int errorIndex) {
- IoTPrinter.print("");
+ // TODO: Not quite sure what this code should actually do
Review Comment:
same as above
##########
iotdb-client/cli/src/main/java/org/apache/iotdb/cli/IoTDBSyntaxHighlighter.java:
##########
@@ -67,12 +67,14 @@ public AttributedString highlight(LineReader reader, String buffer) {
@Override
public void setErrorPattern(Pattern errorPattern) {
- IoTPrinter.print("");
+ // TODO: Not quite sure what this code should actually do
Review Comment:
This class was temporarily disabled in [PR](https://github.com/apache/iotdb/blob/master/iotdb-client/cli/src/main/java/org/apache/iotdb/cli/utils/JlineUtils.java#L90).
Perhaps we can remove this class, or leave it at that? Adding these comments does not seem to help
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/service/DataNode.java:
##########
@@ -917,6 +925,9 @@ public void stop() {
if (dataRegionConsensusStarted) {
DataRegionConsensusImpl.getInstance().stop();
}
+ /*if (IoTDBRestServiceDescriptor.getInstance().getConfig().isEnableRestService()) {
Review Comment:
seems we can remove this line? or just put this uncommented line to `deactivate()`?
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Feat/cli with overridable streams [iotdb]
Posted by "chrisdutz (via GitHub)" <gi...@apache.org>.
chrisdutz commented on PR #11885:
URL: https://github.com/apache/iotdb/pull/11885#issuecomment-1913592945
Hi,
yeah ... it surely doesn't address all singletons, just the ones I needed for my work. Of course adjusting all singletons is the more complete work, but I wanted to change as little as possible.
In general do I think we should completely get rid of any form of static variables for anything but constant definitions.
Currently I would be in strong favor of changing things to compose things using constructors, similar to Spring Constructor Injection, but to manually weave the components together, so we can generally use IoTDB in any form of application, no matter the component framework being used.
However do I know this is a big change and possibly one that could break compatibility with extensions built for older versions, so I think we should do the "resettable statics" first and focus on a more general refactoring later on.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org