Skip to content
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

Introduce module for compat code #18

Merged
merged 4 commits into from
Mar 11, 2020

Conversation

jakelandis
Copy link

@pgomulka

I am reviewing elastic#53228 and am a bit concerned with how spread out the compat changes are. I would like to suggest trying to encapsulate as much as possible in oss/modules or x-pack/plugin. I got an oss/module started and moved the RestGetAction for the compatibility layer over.

If you agree with this, could you accept this PR (which will update elastic#53228). Then can you move as much of code as possible over to it over to the module ? Ideally 100% of the compat code would be in the plugin, but that is not realistic.

We should be able to move all of the RestHandlers pretty easily (as demonstrated in this PR).
I think the scaffolding/core parts are O.K. to live where ever it makes the most sense, but we should strive (if possible) to encapsulate as much of it in the oss/module even if it means some SPI indirection.
Any code specific to a version should be in the oss/module.
I understand the x-content code (and eventually the transport code) isn't so easy to encapsulate, so in the meantime we can just leave them where they are. I like how they mirror the transport code, but would like it better if the module could contain the business logic applied. I have a couple ideas floating around for how we could do that, but none are very good or well formed (callback ?, x-content fragment registry ?, serialize->deserialize->mutate->serialize) . I will hack on those ideas some soon, or happy to hear any ideas you may have to encapsulate the x-content version specific code.

Included in this PR:

  • synced up with the compat_rest_api branch (sorry for the noise)
  • new oss/modules - with spotless enabled
  • moved the RestGetAction compatiblity to the module
  • removed dead? loggers in the real RestGetAction
  • fixes to allow PreCommit to run sucessfully

@pgomulka
Copy link
Owner

looks good!!

@pgomulka pgomulka merged commit cf1d340 into pgomulka:compat/type-index-get Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants