Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SOLR-16553 and SOLR-16559 #1185

Merged
merged 2 commits into from
Nov 28, 2022
Merged

SOLR-16553 and SOLR-16559 #1185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fba08f4
SOLR-16553: solr.install.dir property not initialized
risdenk Nov 22, 2022
0717921
SOLR-16559: Fix benchmark module warnings/errors
risdenk Nov 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion solr/benchmark/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,6 @@ dependencies {
implementation 'org.quicktheories:quicktheories'
implementation 'org.openjdk.jmh:jmh-core'
implementation 'org.slf4j:slf4j-api'
runtimeOnly 'com.lmax:disruptor'
annotationProcessor 'org.openjdk.jmh:jmh-generator-annprocess'
implementation 'org.slf4j:slf4j-api'
}
19 changes: 12 additions & 7 deletions solr/benchmark/log4j2-bench.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,20 +64,22 @@
</RollingRandomAccessFile>


<RandomAccessFile
<RollingRandomAccessFile
name="RandomCountsFile"
fileName="${sys:solr.log.dir:-build/work/solr-logs}/${sys:solr.log.name:-solr}_random_counts.log">
fileName="${sys:solr.log.dir:-work/solr-logs}/${sys:solr.log.name:-solr}_random_counts.log"
filePattern="${sys:solr.log.dir:-work/solr-logs}/${sys:solr.log.name:-solr}_random_counts.log.%i">
<PatternLayout>
<Pattern>
%maxLen{%d{yyyy-MM-dd HH:mm:ss.SSS} %-5p (%t) [%notEmpty{c:%X{collection}}%notEmpty{ s:%X{shard}}%notEmpty{ r:%X{replica}}%notEmpty{ x:%X{core}}] %c{1.}
%m%notEmpty{ =>%ex{short}}}{10240}%n
</Pattern>
</PatternLayout>
</RandomAccessFile>
<AsyncLogger name="org.apache.solr.bench.BaseBenchState.RandomCounts" level="info" additivity="false">
<AppenderRef ref="RandomCountsFile"/>
</AsyncLogger>

<Policies>
<OnStartupTriggeringPolicy/>
<SizeBasedTriggeringPolicy size="128 MB"/>
</Policies>
<DefaultRolloverStrategy max="10"/>
</RollingRandomAccessFile>
</Appenders>
<Loggers>
<!-- Use <AsyncLogger/<AsyncRoot and <Logger/<Root for asynchronous logging or synchonous logging respectively -->
Expand All @@ -92,6 +94,9 @@
<AsyncLogger name="org.apache.solr.core.SolrCore.SlowRequest" level="info" additivity="false">
<AppenderRef ref="SlowLogFile"/>
</AsyncLogger>
<AsyncLogger name="org.apache.solr.bench.BaseBenchState.RandomCounts" level="info" additivity="false">
<AppenderRef ref="RandomCountsFile"/>
</AsyncLogger>

<AsyncRoot level="info">
<AppenderRef ref="MainLogFile"/>
Expand Down
37 changes: 29 additions & 8 deletions solr/core/src/java/org/apache/solr/core/NodeConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,13 +253,13 @@ public Path getSolrDataHome() {
/**
* Obtain the path of solr's binary installation directory, e.g. <code>/opt/solr</code>
*
* @return path to install dir
* @throws SolrException if property 'solr.install.dir' has not been initialized
* @return path to install dir or null if solr.install.dir not set.
*/
public Path getSolrInstallDir() {
String prop = System.getProperty(SolrDispatchFilter.SOLR_INSTALL_DIR_ATTRIBUTE);
if (prop == null || prop.isBlank()) {
throw new SolrException(ErrorCode.SERVER_ERROR, "solr.install.dir property not initialized");
log.debug("solr.install.dir property not initialized.");
return null;
}
return Paths.get(prop);
}
Expand Down Expand Up @@ -436,8 +436,15 @@ private void setupSharedLib() {
Set<String> libDirs = new LinkedHashSet<>();
libDirs.add("lib");

// Always add $SOLR_TIP/lib to the shared resource loader
libDirs.add(getSolrInstallDir().resolve("lib").toAbsolutePath().normalize().toString());
Path solrInstallDir = getSolrInstallDir();
if (solrInstallDir == null) {
log.warn(
"Unable to add $SOLR_HOME/lib for shared lib since {} was not set.",
SolrDispatchFilter.SOLR_INSTALL_DIR_ATTRIBUTE);
} else {
// Always add $SOLR_TIP/lib to the shared resource loader
libDirs.add(solrInstallDir.resolve("lib").toAbsolutePath().normalize().toString());
}

if (!StringUtils.isBlank(getSharedLibDirectory())) {
List<String> sharedLibs = Arrays.asList(getSharedLibDirectory().split("\\s*,\\s*"));
Expand Down Expand Up @@ -478,16 +485,30 @@ private void addFoldersToSharedLib(Set<String> libDirs) {
private void initModules() {
var moduleNames = ModuleUtils.resolveModulesFromStringOrSyspropOrEnv(getModules());
boolean modified = false;

Path solrInstallDir = getSolrInstallDir();
if (solrInstallDir == null) {
if (!moduleNames.isEmpty()) {
throw new SolrException(
ErrorCode.SERVER_ERROR,
"Unable to setup modules "
+ moduleNames
+ " because "
+ SolrDispatchFilter.SOLR_INSTALL_DIR_ATTRIBUTE
+ " was not set.");
}
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of returning null to handle exceptional cases. Why not catch the exception instead?

And certainly not a fan of silenlty returning like this. At the very least, it should do a check to see if any modules are requested, and throw exception if they are but we cannot load them due to, in this case, a configuration error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think getSolrInstallDir() should return an exception if the property isn't set. Returning null from that method is 100% valid since it is saying "there is no install dir" - initModules and setupSharedLib should handle the case of no install dir. I agree that there should be a message returned in that case if there is no install dir.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janhoy I pushed a change that gives some more detail in the case of modules and shared lib. Let me know if that looks better for you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was better, although we'd probably be better off for all tests to also set solr.install.dir and treat lack thereof as a configuration error. But now we at least report why things are failing if it is not set. I'm ok with this too.

}
for (String m : moduleNames) {
if (!ModuleUtils.moduleExists(getSolrInstallDir(), m)) {
if (!ModuleUtils.moduleExists(solrInstallDir, m)) {
log.error(
"No module with name {}, available modules are {}",
m,
ModuleUtils.listAvailableModules(getSolrInstallDir()));
ModuleUtils.listAvailableModules(solrInstallDir));
// Fail-fast if user requests a non-existing module
throw new SolrException(ErrorCode.SERVER_ERROR, "No module with name " + m);
}
Path moduleLibPath = ModuleUtils.getModuleLibPath(getSolrInstallDir(), m);
Path moduleLibPath = ModuleUtils.getModuleLibPath(solrInstallDir, m);
if (Files.exists(moduleLibPath)) {
try {
List<URL> urls = SolrResourceLoader.getURLs(moduleLibPath);
Expand Down
5 changes: 1 addition & 4 deletions solr/core/src/test/org/apache/solr/core/TestCoreContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,10 +496,7 @@ public void testSolrInstallDir() throws Exception {
Path solrInstallDir = cores.getConfig().getSolrInstallDir();
assertTrue(
"solrInstallDir was " + solrInstallDir,
solrInstallDir != null
&& installDirPath
.toString()
.equals(cores.getConfig().getSolrInstallDir().toString()));
solrInstallDir != null && installDirPath.toString().equals(solrInstallDir.toString()));
// Proves that <solr-install-dir>/lib/jar1.jar is found, and the resource inside available
assertNotNull(cores.getResourceLoader().openResource("solrInstallDirLibResource"));
} finally {
Expand Down