- 
                Notifications
    You must be signed in to change notification settings 
- Fork 25.6k
Support 7x segments as archive in 8x / 9x #119503
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.
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
Support 7x segments as archive in 8x / 9x #119503
Conversation
| Pinging @elastic/es-search-foundations (Team:Search Foundations) | 
| Hi @drempapis, I've created a changelog YAML for you. | 
…hub.com:drempapis/elasticsearch into fix/117042_Support_7x_segments_as_archive_in_8x
        
          
                .../src/main/java/org/elasticsearch/xpack/lucene/bwc/codecs/lucene60/MetadataOnlyBKDReader.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left two minors, LGTM otherwise. This was not an easy one, thanks for bearing with me!
        
          
                ...ava/org/elasticsearch/xpack/lucene/bwc/codecs/lucene86/Lucene86MetadataOnlyPointsReader.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | pointCount = metaIn.readVLong(); | ||
| docCount = metaIn.readVInt(); | ||
|  | ||
| // This code has been introduced to process IndexInput created with Lucene86Codec+ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe explain that we are good ignoring additional fields with older formats, just to leave a trace of our analysis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify this comment and add the context from my previous comment around why we conditionally read, and why nothing fails if you remove the conditional? I can see how we may be asking ourselves this same question in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added this; I will extend it with more words.
 // This code has been introduced to process IndexInput created with Lucene86Codec+. This is not necessary
 // in the read-only version for older formats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an additional pr to address the issues post-merge issues #125666
| Thanks for reviewing this @javanna. I learned a lot of stuff working on this! | 
| 💚 Backport successful
 | 
Added BWCLucene8*Codecs wrapper classes for the lucene8* equivalents. A BWC wrapper is initialized for archive indices and provides read-only capabilities for an index.
| I think that this needs to be backported to 8.x, 8.18 and 8.17 as well, but you'll get conflicts and it may be easier to do it manually. Let me know if you need help. | 
Added BWCLucene8*Codecs wrapper classes for the lucene8* equivalents. A BWC wrapper is initialized for archive indices and provides read-only capabilities for an index.
| if (codec == null) return null; | ||
|  | ||
| return switch (codec.getClass().getSimpleName()) { | ||
| case "Lucene70Codec" -> new BWCLucene70Codec(); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @drempapis I missed this during review, not a huge deal, but wrapping Lucene70Codec is not required here. Lucene70Codec is no longer shipped with Lucene 10.0, we rather ship it, and it's already extending BWCCodec. I would instead check that the codec we get extends from BWCCodec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true; I passed it from the debugger to verify it. I'll create a pr to adjust this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ thanks, ping me for review, it'll be quicker this time :)
Added BWCLucene8*Codecs wrapper classes for the lucene8* equivalents. A BWC wrapper is initialized for archive indices and provides read-only capabilities for an index.
Added BWCLucene8*Codecs wrapper classes for the lucene8* equivalents. A BWC wrapper is initialized for archive indices and provides read-only capabilities for an index.
The correspondence between the ES version, Lucene Version, and Lucene codec is as follows.
Each Codec version has a wrapper class handling the underlying Lucene codec.
Closes #117042