Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
bruno-roustant commented on PR #3185: URL: https://github.com/apache/solr/pull/3185#issuecomment-2663595482 @dsmiley I added CHANGES.txt for review. -- 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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
bruno-roustant commented on code in PR #3185:
URL: https://github.com/apache/solr/pull/3185#discussion_r1958539013
##
solr/core/src/java/org/apache/solr/core/CachingDirectoryFactory.java:
##
@@ -391,25 +393,22 @@ public boolean exists(String path) throws IOException {
public final Directory get(String path, DirContext dirContext, String
rawLockType)
throws IOException {
String fullPath = normalize(path);
+Directory directory;
+CacheValue cacheValue;
synchronized (this) {
if (closed) {
throw new AlreadyClosedException("Already closed");
}
- final CacheValue cacheValue = byPathCache.get(fullPath);
- Directory directory = null;
- if (cacheValue != null) {
-directory = cacheValue.directory;
- }
-
- if (directory == null) {
+ cacheValue = byPathCache.get(fullPath);
+ if (cacheValue == null) {
directory = create(fullPath, createLockFactory(rawLockType),
dirContext);
Review Comment:
Thanks! I add the todo.
--
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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
bruno-roustant merged PR #3185: URL: https://github.com/apache/solr/pull/3185 -- 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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
bruno-roustant commented on code in PR #3185:
URL: https://github.com/apache/solr/pull/3185#discussion_r1958509839
##
solr/core/src/java/org/apache/solr/core/CachingDirectoryFactory.java:
##
@@ -391,25 +393,22 @@ public boolean exists(String path) throws IOException {
public final Directory get(String path, DirContext dirContext, String
rawLockType)
throws IOException {
String fullPath = normalize(path);
+Directory directory;
+CacheValue cacheValue;
synchronized (this) {
if (closed) {
throw new AlreadyClosedException("Already closed");
}
- final CacheValue cacheValue = byPathCache.get(fullPath);
- Directory directory = null;
- if (cacheValue != null) {
-directory = cacheValue.directory;
- }
-
- if (directory == null) {
+ cacheValue = byPathCache.get(fullPath);
+ if (cacheValue == null) {
directory = create(fullPath, createLockFactory(rawLockType),
dirContext);
Review Comment:
I prefer to leave that for another PR (I have another proposal to make
CachingDirectoryFactory support a DelegatingDirectoryFactory).
For the TODO, I'm unsure as I see some usage of the DirContext in
HdfsDirectoryFactory which seems valid (creating either a BlockDirectory or a
HdfsDirectory).
--
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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
bruno-roustant commented on code in PR #3185:
URL: https://github.com/apache/solr/pull/3185#discussion_r1958516767
##
solr/core/src/java/org/apache/solr/handler/admin/api/ReplicationAPIBase.java:
##
@@ -377,7 +380,7 @@ public void write(OutputStream out) throws IOException {
try {
initWrite();
-Directory dir = solrCore.withSearcher(searcher ->
searcher.getIndexReader().directory());
+Directory dir = getDirectory();
Review Comment:
Yes, this is the case in the latest commit of the PR.
--
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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
dsmiley commented on code in PR #3185:
URL: https://github.com/apache/solr/pull/3185#discussion_r1958519813
##
solr/core/src/java/org/apache/solr/core/CachingDirectoryFactory.java:
##
@@ -391,25 +393,22 @@ public boolean exists(String path) throws IOException {
public final Directory get(String path, DirContext dirContext, String
rawLockType)
throws IOException {
String fullPath = normalize(path);
+Directory directory;
+CacheValue cacheValue;
synchronized (this) {
if (closed) {
throw new AlreadyClosedException("Already closed");
}
- final CacheValue cacheValue = byPathCache.get(fullPath);
- Directory directory = null;
- if (cacheValue != null) {
-directory = cacheValue.directory;
- }
-
- if (directory == null) {
+ cacheValue = byPathCache.get(fullPath);
+ if (cacheValue == null) {
directory = create(fullPath, createLockFactory(rawLockType),
dirContext);
Review Comment:
Indeed it's valid for a DirectoryFactory to potentially customize the
Directory for use. But it's wrong to do that *here*, as it's basically a bug
in which the first DirContext will "win" and have it be cached; not later
DirContext needs. This is a bug for HdfsDirectory but the fix is in the design
of CachingDirectoryFactory. *Instead*, the filter method you added should be
overwritten so that the DirectoryFactory can tweak the response for the
DirContext. For Hdfs, that would be unwrapping BlockCache for anything but
DEFAULT. Note HdfsDirectory was removed yesterday.
--
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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
dsmiley commented on code in PR #3185:
URL: https://github.com/apache/solr/pull/3185#discussion_r1958484987
##
solr/core/src/java/org/apache/solr/handler/admin/api/ReplicationAPIBase.java:
##
@@ -377,7 +380,7 @@ public void write(OutputStream out) throws IOException {
try {
initWrite();
-Directory dir = solrCore.withSearcher(searcher ->
searcher.getIndexReader().directory());
+Directory dir = getDirectory();
Review Comment:
I can't tell from the PR but this code will need to ensure we release the
directory.
--
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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
dsmiley commented on code in PR #3185:
URL: https://github.com/apache/solr/pull/3185#discussion_r1958467972
##
solr/core/src/java/org/apache/solr/core/CachingDirectoryFactory.java:
##
@@ -391,25 +393,22 @@ public boolean exists(String path) throws IOException {
public final Directory get(String path, DirContext dirContext, String
rawLockType)
throws IOException {
String fullPath = normalize(path);
+Directory directory;
+CacheValue cacheValue;
synchronized (this) {
if (closed) {
throw new AlreadyClosedException("Already closed");
}
- final CacheValue cacheValue = byPathCache.get(fullPath);
- Directory directory = null;
- if (cacheValue != null) {
-directory = cacheValue.directory;
- }
-
- if (directory == null) {
+ cacheValue = byPathCache.get(fullPath);
+ if (cacheValue == null) {
directory = create(fullPath, createLockFactory(rawLockType),
dirContext);
Review Comment:
In a separate issue/PR, the `create` method should not contain the
dirContext leaving a TODO now. Or if you wish, make that change here; it's
rather internal.
--
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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
bruno-roustant commented on PR #3185: URL: https://github.com/apache/solr/pull/3185#issuecomment-2663047519 The new commit implements option B, which works, and the code remains concise. -- 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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
dsmiley commented on PR #3185: URL: https://github.com/apache/solr/pull/3185#issuecomment-2661224062 (A) seems kinda hard due to various entanglements that seem to assume one cache entry per path or other complexities I see there. At least (B) would actually only need to wrap dirs that are not DEFAULT. Also, the word "unwrap" pre-supposes an implementation detail that the callers should ideally not be exposed to and isn't necessarily accurate. Maybe it'll wrap not unwrap or do both or something new; who knows. Solr code using this should just give this DirContext hint as to what the dir will be used for. That's how it is now, albeit we see a bug and the use-cases (DirContext) listed are insufficient. -- 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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
bruno-roustant commented on PR #3185: URL: https://github.com/apache/solr/pull/3185#issuecomment-2661020011 As I see it, the need to unwrap is based on what the caller will use the Directory for. So Im' not sure putting a lot of logic in CachingDirectory in the best solution. Maybe the option B you propose is ok, or we could simply change the ReplicationAPIBase to always unwrap since it copies without Lucene checksum. -- 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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
bruno-roustant commented on code in PR #3185: URL: https://github.com/apache/solr/pull/3185#discussion_r1957163042 ## solr/core/src/java/org/apache/solr/handler/IncrementalShardBackup.java: ## @@ -145,21 +145,22 @@ protected IncrementalShardSnapshotResponse backup(final IndexCommit indexCommit) details.startTime = Instant.now().toString(); Collection files = indexCommit.getFileNames(); +DirectoryFactory directoryFactory = solrCore.getDirectoryFactory(); Directory dir = -solrCore -.getDirectoryFactory() -.get( -solrCore.getIndexDir(), -DirectoryFactory.DirContext.DEFAULT, -solrCore.getSolrConfig().indexConfig.lockType); +directoryFactory.get( +solrCore.getIndexDir(), +DirectoryFactory.DirContext.DEFAULT, Review Comment: In theversion 2 of the PR, I separated the DirContext (what the directory is) and DirUseContext (what the directory will be used for). I put replication and backup in DirUseContext, so they cannot be passed to DirectoryFactory.get(). ## solr/core/src/java/org/apache/solr/handler/IncrementalShardBackup.java: ## @@ -145,21 +145,22 @@ protected IncrementalShardSnapshotResponse backup(final IndexCommit indexCommit) details.startTime = Instant.now().toString(); Collection files = indexCommit.getFileNames(); +DirectoryFactory directoryFactory = solrCore.getDirectoryFactory(); Directory dir = -solrCore -.getDirectoryFactory() -.get( -solrCore.getIndexDir(), -DirectoryFactory.DirContext.DEFAULT, -solrCore.getSolrConfig().indexConfig.lockType); +directoryFactory.get( +solrCore.getIndexDir(), +DirectoryFactory.DirContext.DEFAULT, Review Comment: In the version 2 of the PR, I separated the DirContext (what the directory is) and DirUseContext (what the directory will be used for). I put replication and backup in DirUseContext, so they cannot be passed to DirectoryFactory.get(). -- 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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
dsmiley commented on code in PR #3185:
URL: https://github.com/apache/solr/pull/3185#discussion_r1957129435
##
solr/core/src/java/org/apache/solr/handler/admin/api/ReplicationAPIBase.java:
##
@@ -246,6 +242,21 @@ protected FileListResponse getFileList(long generation,
ReplicationHandler repli
return filesResponse;
}
+ private Directory getDirectory() throws IOException {
+return solrCore
+.getDirectoryFactory()
+.get(
+solrCore.getNewIndexDir(),
+DirectoryFactory.DirContext.DEFAULT,
Review Comment:
And here; wouldn't we use REPLICATION?
##
solr/core/src/java/org/apache/solr/core/DirectoryFactory.java:
##
@@ -55,10 +55,16 @@ public abstract class DirectoryFactory implements
NamedListInitializedPlugin, Cl
// Absolute.
protected Path dataHomePath;
- // hint about what the directory contains - default is index directory
+ /** Hint about what the directory contains or what the directory will be
used for. */
public enum DirContext {
+/** Default is index directory. */
DEFAULT,
-META_DATA
+/** Directory containing metadata. */
+META_DATA,
+/** Directory used to copy raw files during replication. */
+REPLICATE,
+/** Directory used to copy raw files during backup. */
+BACKUP,
Review Comment:
Okay. I see changing the Lucene checksum to be of the raw bytes appears
impossible because that checksum is not the responsibility of the Directory (if
it was we could tweak it). Instead, most Lucene coded stuff writes it via
CodecUtil.writeFooter
##
solr/core/src/java/org/apache/solr/handler/IncrementalShardBackup.java:
##
@@ -145,21 +145,22 @@ protected IncrementalShardSnapshotResponse backup(final
IndexCommit indexCommit)
details.startTime = Instant.now().toString();
Collection files = indexCommit.getFileNames();
+DirectoryFactory directoryFactory = solrCore.getDirectoryFactory();
Directory dir =
-solrCore
-.getDirectoryFactory()
-.get(
-solrCore.getIndexDir(),
-DirectoryFactory.DirContext.DEFAULT,
-solrCore.getSolrConfig().indexConfig.lockType);
+directoryFactory.get(
+solrCore.getIndexDir(),
+DirectoryFactory.DirContext.DEFAULT,
Review Comment:
Wouldn't we use BACKUP even if we're not sure if the DirectoryFactory is
going to do something based on this?
--
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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
bruno-roustant commented on PR #3185: URL: https://github.com/apache/solr/pull/3185#issuecomment-2660928987 This new commit makes the PR work. I added DirectoryFactory.unwrapFor(Directory, DirUseContext) method, with a context to potentially differentiate replication and backup. -- 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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
bruno-roustant commented on code in PR #3185:
URL: https://github.com/apache/solr/pull/3185#discussion_r1957082788
##
solr/core/src/java/org/apache/solr/core/DirectoryFactory.java:
##
@@ -55,10 +55,16 @@ public abstract class DirectoryFactory implements
NamedListInitializedPlugin, Cl
// Absolute.
protected Path dataHomePath;
- // hint about what the directory contains - default is index directory
+ /** Hint about what the directory contains or what the directory will be
used for. */
public enum DirContext {
+/** Default is index directory. */
DEFAULT,
-META_DATA
+/** Directory containing metadata. */
+META_DATA,
+/** Directory used to copy raw files during replication. */
+REPLICATE,
+/** Directory used to copy raw files during backup. */
+BACKUP,
Review Comment:
Actually this very same question was discussed when I introduced
DelegatingBackupRepository. During backup, two different checksums are verified:
- The Lucene format checksum, to verify the file to backup is not corrupted.
This checksum can only be verified cleartext, so for encryption this means it
is verified by the EncryptionDirectory after decrypting.
- The file transfer checksum, to verify the bytes are identical at the
beginning and at the end of the copy. This checksum is verified by
IncrementalShardBackup, and can be verified on either raw or filtered bytes.
Currently it is on filtered bytes.
I specifically asked if we could skip the Lucene checksum verification in
the case of encryption. But since we want to ensure we do not backup a
corrupted file (which could replace an older valid one), then we have to check
the cleartext bytes.
--
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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
dsmiley commented on code in PR #3185:
URL: https://github.com/apache/solr/pull/3185#discussion_r1957047711
##
solr/core/src/java/org/apache/solr/handler/admin/api/ReplicationAPIBase.java:
##
@@ -377,7 +380,7 @@ public void write(OutputStream out) throws IOException {
try {
initWrite();
-Directory dir = solrCore.withSearcher(searcher ->
searcher.getIndexReader().directory());
+Directory dir = getDirectory();
Review Comment:
I like this change (as we discussed in person)
##
solr/core/src/java/org/apache/solr/core/DirectoryFactory.java:
##
@@ -55,10 +55,16 @@ public abstract class DirectoryFactory implements
NamedListInitializedPlugin, Cl
// Absolute.
protected Path dataHomePath;
- // hint about what the directory contains - default is index directory
+ /** Hint about what the directory contains or what the directory will be
used for. */
public enum DirContext {
+/** Default is index directory. */
DEFAULT,
-META_DATA
+/** Directory containing metadata. */
+META_DATA,
+/** Directory used to copy raw files during replication. */
+REPLICATE,
+/** Directory used to copy raw files during backup. */
+BACKUP,
Review Comment:
I wonder if the EncryptionDirectory could somehow ensure that the checksum
is of the underlying raw data and not the cleartext? Or maybe quite simply we
just need a flag on the DirectoryFactory that advises not to do checksum
verification.
--
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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
bruno-roustant commented on PR #3185: URL: https://github.com/apache/solr/pull/3185#issuecomment-2659768506 Actually in its current state, this PR does not work. The Directory returned by the CachedDirectoryFactory may be different than the Directory cached internally. This results in unclosed directories. One option is to cache using a key including the DirContext. Another option is to rollback the DirContext idea and simply let the caller unwrap the Directory based on its usage. Either by directly calling FilterDirectory.unwrap, or by calling a new method DirectoryFactory.unwrap(Directory). Actually I prefer option two. What do you think @dsmiley ? -- 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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
bruno-roustant commented on code in PR #3185:
URL: https://github.com/apache/solr/pull/3185#discussion_r1956348656
##
solr/core/src/java/org/apache/solr/core/DirectoryFactory.java:
##
@@ -55,10 +55,16 @@ public abstract class DirectoryFactory implements
NamedListInitializedPlugin, Cl
// Absolute.
protected Path dataHomePath;
- // hint about what the directory contains - default is index directory
+ /** Hint about what the directory contains or what the directory will be
used for. */
public enum DirContext {
+/** Default is index directory. */
DEFAULT,
-META_DATA
+/** Directory containing metadata. */
+META_DATA,
+/** Directory used to copy raw files during replication. */
+REPLICATE,
+/** Directory used to copy raw files during backup. */
+BACKUP,
Review Comment:
I separated REPLICATE and BACKUP because backup requires more inner checksum
verifications that may need to differentiate the logic between the two. (this
is the case for encryption where we will need the full-fledged
EncryptionDirectory to access the checksum inside the encrypted Lucene file).
--
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
Re: [PR] SOLR-17671 Replication and Backup use an unwrapped Directory. [solr]
bruno-roustant commented on code in PR #3185:
URL: https://github.com/apache/solr/pull/3185#discussion_r1956345110
##
solr/core/src/java/org/apache/solr/core/CachingDirectoryFactory.java:
##
@@ -421,9 +422,21 @@ public final Directory get(String path, DirContext
dirContext, String rawLockTyp
cacheValue.refCnt++;
log.debug("Reusing cached directory: {}", cacheValue);
}
-
- return directory;
}
+return filterDirectory(directory, dirContext);
+ }
+
+ /**
+ * Potentially filters or unwraps the cached {@link Directory} depending on
the intended use
+ * defined by the {@link DirContext}.
+ */
+ protected Directory filterDirectory(Directory dir, DirContext dirContext) {
Review Comment:
I created a new filterDirectory method for two reasons:
- The key for cached directories does not include the DirContext.
- This method can be overridden if a DirectoryFactory implementation needs
to adapt the filtering logic.
--
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
