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

Add opensearch support #167

Closed
jonhealy1 opened this issue Nov 20, 2023 · 20 comments
Closed

Add opensearch support #167

jonhealy1 opened this issue Nov 20, 2023 · 20 comments

Comments

@jonhealy1
Copy link
Collaborator

No description provided.

@jamesfisher-geo
Copy link
Collaborator

jamesfisher-geo commented Nov 21, 2023

Advice I have seen online suggests to turn on "compatibility mode" in OpenSearch. But I did not have any luck with this approach. I have managed to get OpenSearch working with stac-fastapi-elasticsearch by downgrading elasticsearch[async] from 7.17.9 to 7.13.4, before compatibility checks. This required refactoring the query syntax to an older version. So isn't much use for creating something maintainable on recent releases. For example...

7.17.9

await client.indices.create(
        index=index_by_collection_id(collection_id),
        mappings=ES_ITEMS_MAPPINGS,
        settings=ES_ITEMS_SETTINGS,
        ignore=400,  # ignore 400 already exists code
    )
    await client.close()

7.13.4

await client.indices.create(
        index=index_by_collection_id(collection_id),
        body={"mappings":ES_ITEMS_MAPPINGS,
              "settings": ES_ITEMS_SETTINGS},
        ignore=400,  # ignore 400 already exists code
    )
    await client.close()

As python clients move apart, OpenSearch support would require a separate database_logic.py (and maybe a separate repo?) that is configured to use the opensearch-py library.

@jamesfisher-geo
Copy link
Collaborator

Summarizing @philvarner from the STAC Community Meetup:

Something we should probably do is break apart those implementations so that there is one for each and they share almost everything except they have a wrapper around the clients. Like a stac-fastapi-*search repo with interchangeable OpenSearch and Elasticsearch wrapper modules.

A little bit of work to implement but worth it to make completely compatible.

@jamesfisher-geo
Copy link
Collaborator

Hey @jonhealy1 I wonder if we can merge our work. I have been working on a fork of your add_opensearch branch here: https://github.com/AtomicMaps/stac-fastapi-elasto-opensearch/tree/add-opensearch

@jonhealy1
Copy link
Collaborator Author

Hi that would be really good. What are you thinking for how a User chooses which db they want? I was playing around with a couple of things but don't have anything really good yet. The first thing I thought was just an env variable.

@jonhealy1
Copy link
Collaborator Author

What you have looks really good. I see you're using an env variable to switch too. It's probably the easiest way. I have another branch where I was fooling around with having a separate setup.py for opensearch so we could publish stac-fastapi-opensearch on pypi but I haven't got everything working yet.

@jamesfisher-geo
Copy link
Collaborator

Thanks. Yeah, I could not think of a better way than using an environment variable.

Separate setup.py files sounds good. I am finishing up some debugging then should have a version that passes all the tests up in an hour or so

@jamesfisher-geo
Copy link
Collaborator

Just pushed updates that pass tests on both ES and OS. How would two setup.py files work?

@jonhealy1
Copy link
Collaborator Author

What you have looks really good. I am thinking maybe we should merge what you have and then think about going from there. I am not quite sure how it would work right now.

@jonhealy1
Copy link
Collaborator Author

I have something I am working on now. Three packages. A core package with common code like core.py, then a package each for elasticsearch and opensearch. Basically app.py injects the right DatabaseLogic to the core package. https://github.com/stac-utils/stac-fastapi-elasticsearch/tree/common_core/stac_fastapi

@jonhealy1
Copy link
Collaborator Author

@jamesfisher-gis I tried to add you to a reviewer on this pr - #186. I think you probably need to be added via stac-utils. How do you feel about adding your opensearch changes to this branch?

@jamesfisher-geo
Copy link
Collaborator

Nice. I am not in the stac-utils organization yet. I'm happy to join and help with maintaining this project.

I will submit a PR with my changes in an hour and request to merge into the add-opensearch branch.

@philvarner
Copy link
Collaborator

Nice. I am not in the stac-utils organization yet. I'm happy to join and help with maintaining this project.

let me see what I can do about that.

@jonhealy1
Copy link
Collaborator Author

That would be great. Can you add your changes to this branch instead - https://github.com/stac-utils/stac-fastapi-elasticsearch/tree/common_core. I know it's not ideal but I think it's a really good solution.

@philvarner
Copy link
Collaborator

Nice. I am not in the stac-utils organization yet. I'm happy to join and help with maintaining this project.

let me see what I can do about that.

Invited you with the Maintain permission level, which should allow you do most anything you need to.

@jonhealy1
Copy link
Collaborator Author

@philvarner I still can't add James as a reviewer - maybe it's just a temporary glitch

@jamesfisher-geo
Copy link
Collaborator

@philvarner I still can't add James as a reviewer - maybe it's just a temporary glitch

I just accepted the invite. It should work now

@jonhealy1
Copy link
Collaborator Author

Working now!

@jonhealy1
Copy link
Collaborator Author

I think we should maybe look at merging this before we merge in the opensearch code. #177 What do you think? It would mean updating the opensearch logic

@jonhealy1
Copy link
Collaborator Author

Finishing this opensearch work will be a major accomplishment and we will definitely release a new version of the project.

@jamesfisher-geo
Copy link
Collaborator

I think we should maybe look at merging this before we merge in the opensearch code. #177 What do you think? It would mean updating the opensearch logic

Yeah, sounds good. It looks good to me. I will sync those changes with the add_opensearch tomorrow.

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

No branches or pull requests

3 participants