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

SOLR-15745: Convert create-core v2 API to annotations #450

Merged
merged 2 commits into from Dec 12, 2021

Conversation

gerlowskija
Copy link
Contributor

@gerlowskija gerlowskija commented Dec 8, 2021

Description

Solr currently supports two ways of implementing v2 APIs: an older JSON file ('apispec') based approach, and a new framework that relies on annotations. The code base is a slow transition towards the annotation-based approach, but many APIs remain using the 'apispec' model.

Solution

This PR switches a single API, the "create core" API, over to the new annotation-based framework.

Tests

See V2CoresAPIMappingTest.

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the main branch.
  • I have run ./gradlew check.
  • I have added tests for my changes.

/**
* Unit tests for the /cores APIs.
*
* Note that the V2 requests made by these tests are not necessarily semantically valid. They shouldn't be taken as
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for providing this detail! I tend to look at unit tests as "living documentation" so this is good!

"'async': 'requestTrackingId' " +
"}}");

// TODO Delete apispec file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sure you haven't forgotton about this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

;-). You did complete these two TODO's

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, yeah, these are remnants from a TODO list I wrote myself at some point. Will remove.

@JsonProperty
public Boolean loadOnStartup;

// If our JsonProperty clone was more feature-rich here we could specify the property be called 'transient', but
Copy link
Contributor

@epugh epugh Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we just pick a different parameter name and not use the word transient? I.e, should we go solve this by changing somethign elsewhere so this doesnt' have this issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well that's what I am doing here for the v2 API. The property name in v2 is "isTransient".

The v1 query parameter is still named "transient". And we could change that, but it'd run afoul of backwards compatibility concerns. I'd need an 8.x release to sneak a deprecation into, which doesn't seem super likely at this point.

@gerlowskija gerlowskija merged commit 66c76dd into apache:main Dec 12, 2021
@gerlowskija gerlowskija deleted the SOLR-15745-create-core-api branch December 12, 2021 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants