Adding reader settings for moving fst offheap #601
Conversation
| /** | ||
| * Reader only attributes for this directory. | ||
| */ | ||
| private Map<String, String> readerAttributes; |
There was a problem hiding this comment.
I don't think Directory is the right abstraction to hold this. I suggest we add an overload to DirectoryReader.open that looks like this:
public static DirectoryReader open(final Directory directory, final Map<String, String> attributes) throws IOException;
//
public static DirectoryReader open(final IndexCommit commit, final Map<String, String> attributes) throws IOException;that allows us to have a dedicated map of attributes per open call. Subsequent reopens then inherit the attributes form the reader. You can also then make this a property of DirecotryReader I guess.
There was a problem hiding this comment.
I don't think Directory is the right abstraction to hold this.
Is that because single directory can be opened in multiple contexts?
Subsequent reopens then inherit the attributes form the reader. You can also then make this a property of DirecotryReader I guess.
How is the attribute map passed to SegmentReadState in this approach? Also, IndexWriterConfig need not be changed with this approach, right?
There was a problem hiding this comment.
Is that because single directory can be opened in multiple contexts?
I think there are multiple reasons I think it should be either bound to a single reader or writer. ie two calls to DirectoryReader.open can return readers with different fields on-heap even if they share the same directory.
How is the attribute map passed to SegmentReadState in this approach? Also, IndexWriterConfig need not be changed with this approach, right?
I don't think IWC needs to change. I think such a map can be passed down to SegmentCoreReaders and then passed on via SegmentReadState I am not sure why you think we need to change a lot of things?
There was a problem hiding this comment.
I think such a map can be passed down to SegmentCoreReaders and then passed on via SegmentReadState I am not sure why you think we need to change a lot of things?
Maybe, better way is to add readerAttributes in SegmentInfo, and populate that during StandardDirectoryReader.open?
| * opening the index | ||
| */ | ||
| public IndexWriterConfig setReaderAttributes(Map<String, String> readerAttributes) { | ||
| this.readerAttributes = readerAttributes; |
There was a problem hiding this comment.
this one should be an unmodifiable map, you should also make a copy for the attributes passed in.
There was a problem hiding this comment.
Good point. Will make the change. Thanks!
| // docCount != sumDocFreq implying field is not primary key | ||
| if (clone instanceof ByteBufferIndexInput && this.docCount != this.sumDocFreq) { | ||
|
|
||
| final boolean fstOffHeap = readerAttributes != null && "true".equals(readerAttributes.get(FST_OFFHEAP_ATTRIBUTE)); |
There was a problem hiding this comment.
don't we want to have a per-field setting? I mean something like:
"true".equals(readerAttributes.get("blocktree.fst." + fieldInfo.name + ".offheap"));
this would be more flexible no?
There was a problem hiding this comment.
That was the plan initially, but given that non-PK eligible fields are offheap by default. Having single setting instead of per field setting makes more sense.
If false:
Only non-PK fields offheap
If true:
All fields offheap
There was a problem hiding this comment.
why is this? I mean there might be fields that can easily be off-heap while another one should be on-heap?
There was a problem hiding this comment.
why is this? I mean there might be fields that can easily be off-heap while another one should be on-heap?
Having per field setting is unnecessary cognitive load for the user, given that non PK eligible fields are offheap by default. So, users want PK eligible fields either offheap or onheap.
Initially, I did try it with this patch, but found it to be unnecessary. There can be dynamic fields as well, for which you need special keyword, which needs to account for all combinations of offheap and onheap.
|
I tired a different way today and I wonder what you think of this #606 (note I am currently getting a 404 on it - seems a github issue) |
This change adds setting for moving fst offheap, specifically address JIRA-8671