You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/12/02 19:27:47 UTC
[GitHub] [solr] gerlowskija opened a new pull request, #1210: SOLR-16347: Skip JerseyApp creation when v2 disabled
gerlowskija opened a new pull request, #1210:
URL: https://github.com/apache/solr/pull/1210
https://issues.apache.org/jira/browse/SOLR-16347
# Description
Solr supports a system-property ('disable.v2.api') that can be used to turn off the currently experimental v2 API.
This sysprop should have been used to control the creation of Jersey applications (i.e. skip them when v2 is disabled), but I missed this logic the first time around.
# Solution
This commit addresses this lack so that those not using the v2 APIs needn't spend any CPU initializing these Jersey/JAX-RS objects.
# Tests
Manual testing of the system property flag. Automated tests continue to pass. Ishan's 'solr-bench' benchmark shows the expected performance improvement of not having to create these data structures.
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [ ] I have run `./gradlew check`.
- [ ] I have added tests for my 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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija merged pull request #1210: SOLR-16347: Skip JerseyApp creation when v2 disabled
Posted by GitBox <gi...@apache.org>.
gerlowskija merged PR #1210:
URL: https://github.com/apache/solr/pull/1210
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1210: SOLR-16347: Skip JerseyApp creation when v2 disabled
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1210:
URL: https://github.com/apache/solr/pull/1210#discussion_r1038634363
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -297,6 +297,9 @@ && getZkController().getOverseer() != null
public static final long INITIAL_CORE_LOAD_COMPLETE = 0x4L;
private volatile long status = 0L;
+ public static final boolean IS_V2_ENABLED =
+ !"true".equals(System.getProperty("disable.v2.api", "false"));
Review Comment:
;-) I really need to keep a list of all the things that look odd to me that need refactoring ;-)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on a diff in pull request #1210: SOLR-16347: Skip JerseyApp creation when v2 disabled
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on code in PR #1210:
URL: https://github.com/apache/solr/pull/1210#discussion_r1068540157
##########
solr/core/src/java/org/apache/solr/core/PluginBag.java:
##########
@@ -63,17 +64,20 @@
private final Class<T> klass;
private SolrCore core;
private final SolrConfig.SolrPluginInfo meta;
- private final ApiBag apiBag;
- private final ResourceConfig jerseyResources;
public static class JerseyMetricsLookupRegistry
extends HashMap<Class<? extends JerseyResource>, RequestHandlerBase> {}
+ // Used for v2 API lookup and processing
+ private static final boolean IS_V2_ENABLED =
Review Comment:
I ended up dropping the duplicated static variable.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1210: SOLR-16347: Skip JerseyApp creation when v2 disabled
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1210:
URL: https://github.com/apache/solr/pull/1210#discussion_r1038471290
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -297,6 +297,9 @@ && getZkController().getOverseer() != null
public static final long INITIAL_CORE_LOAD_COMPLETE = 0x4L;
private volatile long status = 0L;
+ public static final boolean IS_V2_ENABLED =
+ !"true".equals(System.getProperty("disable.v2.api", "false"));
Review Comment:
is this really how we do these system properties in Solr? Just sort of funny looking starting with `!"true" ;-)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] noblepaul commented on pull request #1210: SOLR-16347: Skip JerseyApp creation when v2 disabled
Posted by GitBox <gi...@apache.org>.
noblepaul commented on PR #1210:
URL: https://github.com/apache/solr/pull/1210#issuecomment-1385915397
@gerlowskija
we should keep this `disabled` by default so that performance testing is not blocked
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] noblepaul commented on pull request #1210: SOLR-16347: Skip JerseyApp creation when v2 disabled
Posted by GitBox <gi...@apache.org>.
noblepaul commented on PR #1210:
URL: https://github.com/apache/solr/pull/1210#issuecomment-1384816806
>most of the reasons a user might've wanted this back then are still valid today:
@gerlowskija
Well, this was added not because V2 itself was not finished. I was worried that V2 may have a potential security vulnerability or performance degradation
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] epugh commented on a diff in pull request #1210: SOLR-16347: Skip JerseyApp creation when v2 disabled
Posted by GitBox <gi...@apache.org>.
epugh commented on code in PR #1210:
URL: https://github.com/apache/solr/pull/1210#discussion_r1038471896
##########
solr/core/src/java/org/apache/solr/core/PluginBag.java:
##########
@@ -63,17 +64,20 @@
private final Class<T> klass;
private SolrCore core;
private final SolrConfig.SolrPluginInfo meta;
- private final ApiBag apiBag;
- private final ResourceConfig jerseyResources;
public static class JerseyMetricsLookupRegistry
extends HashMap<Class<? extends JerseyResource>, RequestHandlerBase> {}
+ // Used for v2 API lookup and processing
+ private static final boolean IS_V2_ENABLED =
Review Comment:
oh goody, we duplicated this static variable ;-)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] noblepaul commented on pull request #1210: SOLR-16347: Skip JerseyApp creation when v2 disabled
Posted by GitBox <gi...@apache.org>.
noblepaul commented on PR #1210:
URL: https://github.com/apache/solr/pull/1210#issuecomment-1336554029
There is not reason to disable V2 in the first place. It has been there forever and it is performant.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on a diff in pull request #1210: SOLR-16347: Skip JerseyApp creation when v2 disabled
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on code in PR #1210:
URL: https://github.com/apache/solr/pull/1210#discussion_r1068539816
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -297,6 +297,9 @@ && getZkController().getOverseer() != null
public static final long INITIAL_CORE_LOAD_COMPLETE = 0x4L;
private volatile long status = 0L;
+ public static final boolean IS_V2_ENABLED =
+ !"true".equals(System.getProperty("disable.v2.api", "false"));
Review Comment:
I ultimately pulled these checks into a method: `V2ApiUtils.isEnabled()`
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on a diff in pull request #1210: SOLR-16347: Skip JerseyApp creation when v2 disabled
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on code in PR #1210:
URL: https://github.com/apache/solr/pull/1210#discussion_r1038473291
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -297,6 +297,9 @@ && getZkController().getOverseer() != null
public static final long INITIAL_CORE_LOAD_COMPLETE = 0x4L;
private volatile long status = 0L;
+ public static final boolean IS_V2_ENABLED =
+ !"true".equals(System.getProperty("disable.v2.api", "false"));
Review Comment:
Idk - I'm sure there's a handful of ways to do it. Probably the cleanest way would be to put a static utility method in V2ApiUtils that abstracts the actual mechanism away entirely.
Tbh I just copied this line from an existing place that we lookup this sysprop - it is a draft PR after all 😛. I'll clean up the actual lookup a bit as I get this ready to merge.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] gerlowskija commented on pull request #1210: SOLR-16347: Skip JerseyApp creation when v2 disabled
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on PR #1210:
URL: https://github.com/apache/solr/pull/1210#issuecomment-1380917707
> There is not reason to disable V2 in the first place
Noble, you were the one that added this sysprop back in 71abe1?
And as far as I can tell - most of the reasons a user might've wanted this back then are still valid today:
- the v2 code is still evolving and might have avoidable bugs that can be closed off by disabling.
- users configuring security might not want the extra surface area to consider
- those worried about performance might not want to pay the cost of bootstrapping multiple API surfaces at startup
> V2 is not an experimental feature anymore
You're right that it's been around forever, but it's absolutely still an experimental feature as evidenced by [this mailing list discussion](https://lists.apache.org/thread/t342hl7lvt5b4qmb5vrrh7tvmdjlbb22), which resulted in [this JIRA ticket](https://issues.apache.org/jira/browse/SOLR-15780) and the related [note in our ref-guide](https://solr.apache.org/guide/solr/latest/configuration-guide/v2-api.html).
----
If you want to remove the flag, I'd be happy to have that discussion. But while its there and is documented in the ref-guide, it makes sense for it to fully work.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org