dsmiley commented on code in PR #2062:
URL: https://github.com/apache/solr/pull/2062#discussion_r1911520815
##########
gradle/testing/randomization.gradle:
##########
@@ -199,7 +199,12 @@ allprojects {
// Enable security manager, if requested. We could move the selection
of security manager and security policy
// to each project's build/ configuration but it seems compact enough
to keep it here for now.
- if
(Boolean.parseBoolean(testOptionsResolved["tests.useSecurityManager"])) {
+ def useSecurityManager =
Boolean.parseBoolean(testOptionsResolved["tests.useSecurityManager"]);
Review Comment:
@HoustonPutman, I would prefer we have generalized mechanisms instead of
one-off peculiarities, and we have them. See globals.gradle propertyOrDevant
and its friends. Is that stuff sufficient? If we need to toggle settings
globally, we already have gradle.properties
##########
gradle/validation/validate-log-calls.gradle:
##########
@@ -118,7 +118,7 @@ class ValidateLogCallsTask extends DefaultTask {
if (hasPlus) {
cause = "hasPlus: " + hasPlus
- violation = true
+ violation = level != "error"
Review Comment:
What's this about? Could use a comment.
##########
solr/modules/cross-dc/build.gradle:
##########
@@ -0,0 +1,70 @@
+import org.apache.tools.ant.taskdefs.condition.Os
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+apply plugin: 'java-library'
+
+description = 'Cross-DC Replication Plugins'
+
+ext {
+ // Use of the security manager is not supported on windows.
Review Comment:
Why is this concern specific to cross-dc? We use Mockito throughout Solr
tests.
##########
solr/solrj-zookeeper/build.gradle:
##########
@@ -37,7 +37,7 @@ dependencies {
implementation 'org.apache.httpcomponents:httpclient'
implementation 'org.apache.httpcomponents:httpcore'
- implementation('org.apache.zookeeper:zookeeper', {
Review Comment:
Why was this done? It suggests any consumer of solrj-zookeeper would likely
want to use ZK APIs directly. But I don't think that's true.
##########
solr/test-framework/src/java/org/apache/solr/SolrTestCaseJ4.java:
##########
@@ -2158,7 +2158,8 @@ public static File getFile(String name) {
return file;
}
throw new RuntimeException(
- "Cannot find resource in classpath or in file-system (relative to
CWD): " + name);
+ "Cannot find resource in classpath or in file-system (relative to
CWD): "
Review Comment:
shouldn't the exception have the name as-given.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
