You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by franz1981 <gi...@git.apache.org> on 2016/12/15 14:22:22 UTC
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
GitHub user franz1981 opened a pull request:
https://github.com/apache/activemq-artemis/pull/924
ARTEMIS-878: Fixed Boolean deleteOnNoConsumer CLI command property
Fixed a Boolean CLI command property to be used as a nullable property and not as a boolean one by declaring it as a String and parsing/encoding it manually.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/franz1981/activemq-artemis commands_cli_fix
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/924.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #924
----
commit 1bb2fec86191fb6602b0cfc530e1b76cce72ba7a
Author: Francesco Nigro <ni...@gmail.com>
Date: 2016-12-15T14:13:50Z
ARTEMIS-878: Fixed Boolean deleteOnNoConsumer CLI command property
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92660504
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
Hence you are suggesting to have:
1) --deleteOnNoConsumers to set the property to true
2) --noDeleteOnNoConsumers to set the property to false
3) nothing, to not update the property
right?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92668152
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
@clebertsuconic You only get -1 like every one else ;)
TBH I care less about the mechanism and more about the abiliity to update a queue. If there's precedence for this case already set, then fair enough. @franz1981 probably best to be consistent for the time being. I would like to see more effort put into the CLI in general, IMO it needs some care and attention, but we can address that at a later date.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92696195
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
I will send a PR soon.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis issue #924: ARTEMIS-878: Fixed Boolean deleteOnNoConsumer C...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/924
working on this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92655684
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
I need a parameter with 3 possible values:
- null: no update to the queue property will happen
- true: the queue property will be updated using a value of true
- false: the queue property will be updated using a value of false
I know that is something different, but it's a different use case..
In the original version of UpdateQueue I've declared it as Boolean, but after creating a queue with
deleteOnNoConsumers=true there are no ways to use an UpdateQueue command to turn deleteOnNoConsumers to false.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92656577
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
-1000 Martyn. All the Boolean are like this.
I would even make the use of inputs here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 closed the pull request at:
https://github.com/apache/activemq-artemis/pull/924
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92657294
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
if you specify it you've to put a value for it, but if you do not specify it is null (and not updated).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92696163
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
look at Create, allow-anonymous | --require-login
(Remember when I talked about being creative ? :) )
anyways.. I was looking into this, and I found a better way to address this, by using inputs on certain defaults.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92657985
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
I had a similar case somewhere else. Where I created two different Booleans. And it would be an error to have both.
--optionA or --noOptionA
The input could then be used if none is used.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92663530
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
I understand...
It is a personal taste, but it is not counterintuitive to have 2 different (and long) properties to be remembered instead of the same?
In addition, consider for example a case in which there is already a 'no' prefix on a property name; the negation of it will risk (depends) to become something like --noNoXXX.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92661651
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
That's in line to other places on the CLi.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92678031
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
Yes, thanks guys for the comments, I'll make ASAP another PR with the change suggested :+1:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92649779
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
Look at create, it's doing the same way...
Even better.. look at any boolean arguments on the CLI. We need to be consistent here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by mtaylor <gi...@git.apache.org>.
Github user mtaylor commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92656323
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
There's an underlying problem with this approach. The presence of an argument represents true and the non presence represents false. But what if the field is optional? Does that imply false? Specifically if you want to update say a queue by setting "deleteOnNoConsumers" to a different value that is already set, how do we distinguish whether or not the user wants to set it to false, or the user does not want to update it?
IMO it's better to be explicit about options like this. If we really want to keep the presence as an option then perhaps we can add another flag, enableDeleteOnNoConsumers, IMO this is less usable than just adding an option for "true" or "false" to be passed in.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92684194
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
Yes, I'm reusing this branch :+1:
Right now, I'm searching for an already existing example on CLI commands to look on how it is implemented (and use the same name pattern) as a reference, but i haven't found it yet, do you know where it is?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92667568
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
You need to be creative with these names to avoid the -noNo case or course.
It is the framework we have at the moment.
Whatever you do it needs consistency.
Talking about consistency the exceptions need to show help errors and these addresses are not working on that sense. I will make some fixes next week.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92656378
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
Just make it true or false every time. And make it mandatory.
The three arguments make it confusing for the user.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] activemq-artemis pull request #924: ARTEMIS-878: Fixed Boolean deleteOnNoCon...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/924#discussion_r92649677
--- Diff: artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/queue/UpdateQueue.java ---
@@ -31,7 +31,7 @@
String name;
@Option(name = "--deleteOnNoConsumers", description = "whether to delete when it's last consumers disconnects)")
- Boolean deleteOnNoConsumers = null;
+ String deleteOnNoConsumers = null;
--- End diff --
That's wrong...
Boolean arguments are just set with --deleteOnNoConsumers
No arguments...
By doing this you will have to specify the type... which is different from anything on the CLi.. that's wrong..
The CLI is totally broken BTW in regards to address and queues... the errors are not displayed correctly... most of the other CLI will use input on key elements.
This needs to be addressed somehow.. but this PR is not consistent with the other operations.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---