SOLR-15060: Introduce DelegatingDirectoryFactory. #2166
Conversation
| maxWriteMBPerSecFlush = (Double) args.get("maxWriteMBPerSecFlush"); | ||
| maxWriteMBPerSecMerge = (Double) args.get("maxWriteMBPerSecMerge"); | ||
| maxWriteMBPerSecRead = (Double) args.get("maxWriteMBPerSecRead"); | ||
| maxWriteMBPerSecDefault = (Double) args.get("maxWriteMBPerSecDefault"); | ||
|
|
There was a problem hiding this comment.
I simply noticed that those fields were private and actually never used. I think they should be removed for clarity, but that's not necessary for this PR.
There was a problem hiding this comment.
I looked at these a week ago. It's lingering left-over stuff from a refactoring McCandless did years ago in where some thresholds are configured.
| * Configured with the {@code delegateFactory} parameter which defines the full class name of the delegate | ||
| * {@link DirectoryFactory}, plus any additional parameter for the delegate factory.</p> | ||
| */ | ||
| public class DelegatingDirectoryFactory extends DirectoryFactory { |
There was a problem hiding this comment.
Good point, yes it misses abstract.
| */ | ||
| @Override | ||
| public final Directory get(String path, DirContext dirContext, String rawLockType) | ||
| public final Directory get(String path, DirContext dirContext, String rawLockType, Function<Directory, Directory> wrappingFunction) |
There was a problem hiding this comment.
is cachingdirectoryfactory supposed to extend delegatingDF now? I'm confused about the relationship here.
There was a problem hiding this comment.
I don't think CachingDF should extend DelegatingDF. Here the goal with this new param is to allow the caller to have a hook when the Directory is created internally. Otherwise there is no way a DelegatingDF "A" can delegate to any CachingDF "B" while leveraging the cache inside B and at the same time have control over the class of Directory cached and returned.
Another option could be to first extend the CachingDF with a B' class that overrides the createDirectory() method. But that means a DelegatingDF can only delegate to a specific class B' extending CachingDF. This would be too limiting.
There was a problem hiding this comment.
A sample usage could be a BlobDF extending DelegatingDF to delegate to any configured DF, probably MMapDF which is a CachingDF. BlobDF would need to create BlobDir that delegates to a MMapDir. BlobDir cannot wrap the MMapDir outside of the CachingDF, because for the cache to actually work, it has to cache the BlobDir instance. That's why the idea is to provide a wrapper function to wrap a MMapDir with a BlobDir when it is created internally by the CachingDF.
|
The PR is about introducing a low level utility that we expect will be useful for custom DirectoryFactory implementations that we don't even have yet. It's the kind of thing that many of us wouldn't of even created a separate PR for -- it'd show up in a larger PR that introduces the first user of the utility. It's not even worth a CHANGES.txt entry IMO; there's always the commit message. As long as there is no user of this code yet, I'd prefer that we don't merge this yet. Perhaps we may find that the abstraction here isn't quite right. RE master (9) vs 8x... If we wait till BlobDirectory, it'd almost certainly be 9. If we don't, ehh... I'd just do 9. |
|
Actually I'll need that to simplify the code in BlobDirectory. So based on your remark David, I should rather integrate this code in BlobDirectory code and close this Jira issue. |
|
An in-between is to leave this issue open until the BlobDirectory is much more ready. It may change in the mean time; who knows. I do like the notion of committing separate pieces instead of one feature! |
For the description, see https://issues.apache.org/jira/browse/SOLR-15060
This PR adds a new method get(String path, DirContext dirContext, String rawLockType, Function<Directory, Directory> wrappingFunction) in DirectoryFactory, with a new wrappingFunction parameter. The existing get(String path, DirContext dirContext, String rawLockType) calls the new one with Function.identity().
I wonder if this change should be 8x or 9x.