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 API which allows resolving clusters from a given command criterion #956

Closed
wants to merge 2 commits into from

Conversation

tgianos
Copy link
Contributor

@tgianos tgianos commented Feb 21, 2020

/api/v3/commands/{id}/clusterCiteria/{priority}/clusters will execute the criterion resolution against the clusters in the database to see what would be selected if a job was submitted and evaluated against that criterion

`/api/v3/commands/{id}/clusterCiteria/{priority}` will execute the criterion resolution against the clusters in the database to see what would be selected if a job was submitted and evaluated against that criterion
@tgianos tgianos added this to the 4.0.0 milestone Feb 21, 2020
@tgianos tgianos requested a review from mprimi February 21, 2020 23:27
@tgianos tgianos self-assigned this Feb 21, 2020
@mprimi
Copy link
Contributor

mprimi commented Feb 21, 2020

I'm not in favor of indroducing new API that solves a thin sliver of a problem (but that we will have to maintain or worry about breaking going forward).

It seems to me that this exposes something that we can already kinda do via search APIs.

I'm in favor of introducing API if it solves the whole problem.
In my head, solving this entire problem means

  • Take (job) command criteria and (job) cluster criteria
  • Expose all steps and intermediate matching result

This is so that when something is not working, we can take a job, run it through this API and have a full debug trace.

@coveralls
Copy link

coveralls commented Feb 22, 2020

Coverage Status

Coverage decreased (-0.002%) to 92.86% when pulling 561f743 on tgianos:GENIE-374 into b9581ee on Netflix:master.

@codecov
Copy link

codecov bot commented Feb 22, 2020

Codecov Report

Merging #956 into master will not change coverage.
The diff coverage is 80%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #956   +/-   ##
========================================
  Coverage        90%     90%           
- Complexity     3613    3615    +2     
========================================
  Files           439     439           
  Lines         14220   14230   +10     
  Branches        936     937    +1     
========================================
+ Hits          12798   12807    +9     
- Misses         1016    1017    +1     
  Partials        406     406
Impacted Files Coverage Δ Complexity Δ
...pis/rest/v3/controllers/CommandRestController.java 85.56% <80%> (-0.32%) 46 <1> (+1)
...services/jpa/JpaClusterPersistenceServiceImpl.java 92.3% <0%> (+0.51%) 58% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9581ee...561f743. Read the comment docs.

@tgianos
Copy link
Contributor Author

tgianos commented Feb 24, 2020

@mprimi I believe you're discussing a different problem than this is targeted at. Your vision for helping to debug execution details is valid and useful but this specific PR is intended for helping administrators more so than end users.

The admins of our deployment asked for the ability to tell which clusters a command can run on and which commands a cluster can use during our meeting last week. I circled back with those admins and asked specifically about this PR and they re-iterated that this seems to solve what they're asking for as they work on the migration from old to new resource resolution mechanisms. Yes, this can partially be addressed with the search API but it is not abstracted in the same manner and isn't as specifically targeted to recreate the environment the clusters are resolved in as this API is.

It builds on existing APIs in the service tier developed for this brand new cluster/command resolution feature so maintenance going forward shouldn't be an issue as it is where we're heading in the near term.

@tgianos
Copy link
Contributor Author

tgianos commented Feb 27, 2020

After discussion on design I'm going to revise this a little bit and open a new PR.

@tgianos tgianos closed this Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants