warperwolf commented on a change in pull request #324:
URL: https://github.com/apache/solr/pull/324#discussion_r790769421
##########
File path: solr/core/src/java/org/apache/solr/update/UpdateHandler.java
##########
@@ -115,12 +115,9 @@ public UpdateHandler(SolrCore core, UpdateLog updateLog) {
boolean skipUpdateLog = core.getCoreDescriptor().getCloudDescriptor() !=
null && !core.getCoreDescriptor().getCloudDescriptor().requiresTransactionLog();
if (updateLog == null && ulogPluginInfo != null &&
ulogPluginInfo.isEnabled() && !skipUpdateLog) {
DirectoryFactory dirFactory = core.getDirectoryFactory();
- if (dirFactory instanceof HdfsDirectoryFactory) {
- ulog = new
HdfsUpdateLog(((HdfsDirectoryFactory)dirFactory).getConfDir());
- } else {
- ulog = ulogPluginInfo.className == null ? new UpdateLog():
- core.getResourceLoader().newInstance(ulogPluginInfo,
UpdateLog.class, true);
- }
+
+ ulog = ulogPluginInfo.className == null ? new UpdateLog():
Review comment:
I had a discussion with @risdenk on this one. Indeed the code currently
picks the standard UpdateLog.class unless the sorlconfig.xml explicitely
specifies the class of the updateLog definition. This can lead to the update
logs being written to the local FS while the index of the same collection is
being written to HDFS. While this is technically possible (it worked during my
basic tests), this is likely not what we want.
The Solr tests use the solr.tests.ulog variable for this, I took care of the
correct setting for Hdfs tests.
<updateLog class="${solr.tests.ulog:solr.UpdateLog}">
This should be documented.
With Kevin we also discussed that we could look into the Hdfs directory
factory intialization and verify if we can check if an inconsistent update log
class is used and error out in this case. I checked the directory factory code
and to me it seems that we have no access to the update log type there, likely
because they can also be defined per update handler and initialized later.
I think we could also consider the following: add a new method like
getDefaultUpdateLogClassName to the DirectoryFactory class. This method which
would return "solr.UpdateLog" in the base class, and the HdfsDirectoryFactory
could override this by returning "solr.HdfsUpdateLog". In the UpdateHandler
part, if we detect that the solrconfig did not specify any class, we could fall
back to the one returned by this method. This would ensure compatibility with
the existing solrconfigs for HDFS and non-HDFS configs (less pain during an
upgrade), and at the same time it would allow the user to override the value in
SolrConfig. @risdenk , @dsmiley what do you think?
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
