opensearch-project / custom-codecs

OpenSearch custom lucene codecs for providing different on-disk index encoding (e.g., compression).
Apache License 2.0
6 stars 13 forks source link

Custom Codecs Upgrade to Lucene99 Codec #95

Closed sarthakaggarwal97 closed 5 months ago

sarthakaggarwal97 commented 5 months ago

Description

This change would upgrade the underlying Lucene95 codec of the custom codecs to Lucene99 codec.

Issues Resolved

94

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

sarthakaggarwal97 commented 5 months ago

@reta @backslasht @mgodwan please take a look. Working simultaneously on the upgrade ITs to ensure correctness.

reta commented 5 months ago

I think we would need to have a BWC tests here: the codecs created by previous versions of the plugin could be picked up by the new versions.

sarthakaggarwal97 commented 5 months ago

@reta makes sense. Have moved the lucene95 codecs to a backward_codecs directory. While I'm looking to introduce the bwc tests in this plugin, do you think we should take the tests in a separate PR or in the same one?

reta commented 5 months ago

While I'm looking to introduce the bwc tests in this plugin, do you think we should take the tests in a separate PR or in the same one?

I would feel more confident having the change and the tests together, the is a risk (sadly, always) to break on upgrades, thanks @sarthakaggarwal97

sarthakaggarwal97 commented 5 months ago

@reta @msfroh @mgodwan I'm trying to introduce the support for bwc tests in custom-codecs plugin, based on the implementation of security plugin, but looks like I'm running into the same issue for some time now.

It would be great if I can get some inputs on this draft change, since we need to close on this before v2.12 release. Here is the link for the failed bwc task for reference.

Thank you!

mgodwan commented 5 months ago

I understand that currently 2.12 build is failing as we don't have the required custom codecs change in. Due to this, the bwc tests are being run against 2.11 which is not a wire compatible version for main/3.0

This would cause any mixed mode to fail in your tests today.

This is a raw PR which runs against 2.12 as the current version and verify its bwc (at least from a cluster run perspective) with 2.11: https://github.com/opensearch-project/custom-codecs/pull/98/

In order to have a path forward, I think we can try following approaches:

  1. Raise a PR directly against 2.x to fix the 2.12 branch. Raise the same PR with required bwc versions in main
  2. Raise a PR in main with failing bwc tests. Backport to 2.x and then fix the main branch code.

@reta @sarthakaggarwal97 Thoughts?

sarthakaggarwal97 commented 5 months ago

@mgodwan thank you for your inputs. It looks like the minimum_wire_compatibility_version was the issue all along. Option 2 looks good to me (with monitoring that, post merge to 2.x, things still look okay), not sure we follow the practice of merging in 2.x prior to main when it comes to major code changes.

@reta what would you suggest?

reta commented 5 months ago

@reta what would you suggest?

Sorry @sarthakaggarwal97 , just got to that, looking into the options

reta commented 5 months ago

@reta what would you suggest?

Sorry @sarthakaggarwal97 , just got to that, looking into the options

@sarthakaggarwal97 I think we may go 2 step processes, @mgodwan @msfroh please validate me here: 1) Update 2.x (2.12.0-SNAPSHOT) to reflect Apache Lucene 9.9.1 update: at this moment we would use backward_codecs for Lucene 9.5, that would bring 2.x back on track. 2) Move on with BWC tests as planned

Alternatively, we could do 2nd option (as @mgodwan suggested) and have 2 pull requests, I think it would be better and less cycles wasted:

2) Raise a PR directly against 2.x to fix the 2.12 branch. Raise the same PR with required bwc versions in main

What do you think?

msfroh commented 5 months ago

What do you think?

I think @mgodwan's suggestion is probably smoother (less of a BWC dance). While we don't normally like to commit directly to 2.x, I think it's reasonable to unblock us here.