Prevent conflicting connection flags from being used via OptionGroup

[epugh]

https://issues.apache.org/jira/browse/SOLR-XXXXX

Description

Please provide a short description of the changes you're making with this pull request.

Solution

Please provide a short description of the approach taken to implement your solution.

Tests

Please describe the tests you've developed or run to confirm this patch implements the feature or solves the problem.

Checklist

Please review the following and check all that apply:

• [ ] I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
• [ ] I have created a Jira issue and added the issue ID to my pull request title.
• [ ] I have given Solr maintainers access to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
• [ ] I have developed this patch against the main branch.
• [ ] I have run ./gradlew check.
• [ ] I have added tests for my changes.
• [ ] I have added documentation for the Reference Guide

[malliaridis]

@epugh I think I don't have the permissions to directly push to it. But I have done some untested work how we could further improve the implementation with OptionGroups (and there may be more optimizations now possible).

Have a look at https://github.com/epugh/solr/compare/experiment_option_group...malliaridis:solr:experiment_option_group

If you want to add the changes to your branch I can create a PR, just let me know. :)

I also found a few cases where deprecated options are no longer supported in this branch, so I might file a ticket for bringing back support in 9.8 (just in case) if this is an issue there.

[epugh]

@epugh I think I don't have the permissions to directly push to it. But I have done some untested work how we could further improve the implementation with OptionGroups (and there may be more optimizations now possible).

Have a look at epugh/solr@experiment_option_group...malliaridis:solr:experiment_option_group

If you want to add the changes to your branch I can create a PR, just let me know. :)

I also found a few cases where deprecated options are no longer supported in this branch, so I might file a ticket for bringing back support in 9.8 (just in case) if this is an issue there.

This is really cool... I just added you to my epugh:solr repo, hopefully I did it right!

My head is thinking that OptionGroups, and using the explicit objects instead of strings is the way to go. In terms of timing, I'm sort of thinking that maybe this all goes in 10 (main) only. I'm really looking forward to getting rid of all the deprecated code, which will simplify a lot of the complex logic on params, simplify our testing, and open the door to more refactorings like embracing OptionGroups. WDYT? I don't believe 10 will be that far off anyway, so if main and branch_9x diverage a bit, that is okay. Maybe we open a draft PR for OptionGroup in 10, and push hard to finish the conflicting params work that is in 10 and 9, and then finish off the OptionGroup work?

[malliaridis]

My head is thinking that OptionGroups, and using the explicit objects instead of strings is the way to go.

I agree. Referencing OptionGroup and Option objects prevent us from searching explicitly each class for String occurences, which makes maintenance and deprecation much easier in the long run, and deprecated options won't be overseen.

In terms of timing, I'm sort of thinking that maybe this all goes in 10 (main) only.

Yeah, I think introducing such changes in 9.X would be unnecessary now if 10.0 is already in sight.

I'm really looking forward to getting rid of all the deprecated code, which will simplify a lot of the complex logic on params, simplify our testing, and open the door to more refactorings like embracing OptionGroups.

I tried to make commits that would be theoretically easier backported, but at the current state I don't think that is an easy task anymore.

[malliaridis]

@epugh this one is ready I believe. The merge with main was ugly. >.<

I ended up with more lines added than removed, even though I have simplified and removed redundant elements. This is probably only because of the extraction of options to a separate file or variables.

I made a few cleanups and migrations for consistency and also fixed a bug in StatusTool (see 31b1becd357e3f6d22950e9868cd1ff07686b129). Now we are using getOptionValue only with Option as parameter, not strings. And I also migrated to getParsedOptionValue wherever possible (booleans are not supported and the File parsing seems buggy).

[malliaridis]

I didn't find any options that are mutually exclusive to use groups. This was relevant for the deprecated options to simplify the getOptionValue with a single option group. But since we have removed the deprecated options, we are back again using only Options.

[epugh]

I go back and forth on making everything a Java object. cli.getOption("my-option") to me reads better than cli.getOption(MY_OPTION), having said that, I think the enhanced IDE integration is the way to go...! Glad to see the tests get fixed! I will review in the AM and merge.

Is this for branch_9x as well???

[malliaridis]

I go back and forth on making everything a Java object. cli.getOption("my-option") to me reads better than cli.getOption(MY_OPTION)

I agree on that, but I also feel that having long and short variants of options makes `cli.getOption("my-option") kinda confusing. And it has proven to be also error-prone when working with strings. Forgetting to update a referene is easier with strings than with object references. This is probably the most important reason to go for objects.

Is this for branch_9x as well???

No, 9x would require all the deprecated options as well, resulting to OptionGroups. If we make a completely different PR we could introduce similar changes, but backporting won't do here withot breaking backwards compatibility.

[epugh]

Okay, I think this may be finally ready to merge to main (only).