You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "EdColeman (via GitHub)" <gi...@apache.org> on 2023/05/31 17:53:55 UTC
[GitHub] [accumulo] EdColeman opened a new pull request, #3444: Remove the deprecated -s (safemode) option from ConfigOpts
EdColeman opened a new pull request, #3444:
URL: https://github.com/apache/accumulo/pull/3444
The ConfigOpts used -s option for safemode. Remove the deprecated option to allow utilities to reuse ConfigOpts and use -s for set to match the shell option.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #3444: Remove the deprecated -s (safemode) option from ConfigOpts
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3444:
URL: https://github.com/apache/accumulo/pull/3444#issuecomment-1574135412
I think that the fact that it originated from `GCOpts` makes it much much less of an issue. I'm good with 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3444: Remove the deprecated -s (safemode) option from ConfigOpts
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3444:
URL: https://github.com/apache/accumulo/pull/3444#issuecomment-1574129199
> > I asked for @dlmarion 's re-review on this, because he had a different view than my own about what should be done here, and I'm not sure what he thinks about the updates/conversation since his earlier review.
>
> I provided my opinion. Others had a different opinion, I'm good with it.
Okay, I just wanted to check in to see if maybe your opinion changed with the new info about it originating from GCOpts, being hidden from users in ConfigOpts, and the default error message / usage when attempting to use a legacy option. I thought maybe those added details might have convinced you that we don't actually need to keep this around and add your alternate solution. I'd rather get consensus than try to outvote you, at the very least, as a sanity check for myself that I'm not missing something important that will negatively impact users.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #3444: Remove the deprecated -s (safemode) option from ConfigOpts
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii merged PR #3444:
URL: https://github.com/apache/accumulo/pull/3444
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #3444: Remove the deprecated -s (safemode) option from ConfigOpts
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3444:
URL: https://github.com/apache/accumulo/pull/3444#issuecomment-1572732047
I made a comment in #3445 as to what I was proposing.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3444: Remove the deprecated -s (safemode) option from ConfigOpts
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3444:
URL: https://github.com/apache/accumulo/pull/3444#issuecomment-1573931454
I asked for @dlmarion 's re-review on this, because he had a different view than my own about what should be done here, and I'm not sure what he thinks about the updates/conversation since his earlier review.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3444: Remove the deprecated -s (safemode) option from ConfigOpts
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3444:
URL: https://github.com/apache/accumulo/pull/3444#issuecomment-1572691604
The current ConfigOpts parseArgs handles retrieving the configuration and is very useful and is the reason to extend that class in the first place. While that functionality could refactored to be made available, it seems unnecessary for something that is deprecated and can be removed.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #3444: Remove the deprecated -s (safemode) option from ConfigOpts
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3444:
URL: https://github.com/apache/accumulo/pull/3444#issuecomment-1572703716
> While that functionality could refactored to be made available, it seems unnecessary for something that is deprecated and can be removed.
But it was deprecated in `2.1.0` from what I can tell. The deprecation cycle is not very long if we remove it in `2.1.1`. My concern would be that someone is using `-s` currently with `1.10.x` and an upgrade breaks them. Don't we typically follow the semver deprecation strategy even when the item is not in the public api?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3444: Remove the deprecated -s (safemode) option from ConfigOpts
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3444:
URL: https://github.com/apache/accumulo/pull/3444#issuecomment-1573929675
> It prints the help and halts.
Awesome! I think that's perfect, then.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3444: Remove the deprecated -s (safemode) option from ConfigOpts
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3444:
URL: https://github.com/apache/accumulo/pull/3444#issuecomment-1573817264
1dfb3487fd completely removes the legacy option as suggested by @ctubbsii
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3444: Remove the deprecated -s (safemode) option from ConfigOpts
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3444:
URL: https://github.com/apache/accumulo/pull/3444#issuecomment-1573900459
It prints the help and halts.
```accumulo zoo-prop-set-tool --foo
Usage: org.apache.accumulo.server.conf.util.ZooPropSetTool [options]
Options:
...
Was passed main parameter '--foo' but no main parameter was defined in your arg class
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] EdColeman commented on pull request #3444: Remove the deprecated -s (safemode) option from ConfigOpts
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3444:
URL: https://github.com/apache/accumulo/pull/3444#issuecomment-1570669147
The use of -s (set) change will allow the utility to set ZooKeeper properties for issue #3423 to match the shell `config -s` option.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #3444: Remove the deprecated -s (safemode) option from ConfigOpts
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3444:
URL: https://github.com/apache/accumulo/pull/3444#issuecomment-1573958720
> I asked for @dlmarion 's re-review on this, because he had a different view than my own about what should be done here, and I'm not sure what he thinks about the updates/conversation since his earlier review.
I provided my opinion. Others had a different opinion, I'm good with 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #3444: Remove the deprecated -s (safemode) option from ConfigOpts
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #3444:
URL: https://github.com/apache/accumulo/pull/3444#issuecomment-1572679217
Since this deprecation change was made in 2.1, and you want to remove / repurpose `-s` in 2.1.1, I wonder if there is a more friendly way to handle this. For example, if your utility had it's own Opts class that extended ConfigOpts or ServerOpts, then you could override ConfigOpts.parseArgs and neuter the legacy option processing.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org