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

[ResourceCluster] Add basic host routes to v1 API. #197

Merged
merged 6 commits into from
Apr 25, 2022

Conversation

Andyz26
Copy link
Collaborator

@Andyz26 Andyz26 commented Apr 22, 2022

Context

Add API for ResouceClustersHostActor public facing operations.

Checklist

  • ./gradlew build compiles code correctly
  • Added new tests where applicable
  • ./gradlew test passes all tests
  • Extended README or added javadocs where applicable
  • Added copyright headers for new files from CONTRIBUTING.md

@github-actions
Copy link

github-actions bot commented Apr 22, 2022

Unit Test Results

113 files  ±0  113 suites  ±0   5m 25s ⏱️ -1s
483 tests ±0  463 ✔️ ±0  20 💤 ±0  0 ±0 

Results for commit 0ee21ae. ± Comparison against base commit 96d0f24.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@codyrioux codyrioux left a comment

Choose a reason for hiding this comment

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

LGTM!

public class ResourceClustersPayloads {
public static final String CLUSTER_ID = "mantisResourceClusterUT1";

public static final String RESOURCE_CLUSTER_CREATE = "{\"clusterId\":\"mantisResourceClusterUT1\",\"clusterSpec\":"
Copy link
Contributor

Choose a reason for hiding this comment

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

More of a personal preference than anything, but these may be painful to modify for testing if the data structure changes in any sort of meaningful way. Some kind of structure that gets serialized, or even a well formatted ".json" file that is read in would be easier to edit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

+1. Moved to a resource JSON file.

import io.mantisrx.control.plane.resource.cluster.proto.ScaleResourceResponse;
import java.util.concurrent.CompletionStage;

public interface ResourceClusterRouteHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

I may have missed some team discussion on this but do we have an official stance on interface names I ResourceClusterRouteHandler vs ResourceClusterRouteHandler? I don't have a strong opinion either way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was brought up a few times in some of the previous PR comments from @sundargates IIRC and I am following his pattern in the new OSS mantis code at this point to be consistent.

.mapAsync(1, rc -> updateRegisteredClusters(rc))
.mapAsync(1, notUsed -> getRegisteredResourceClustersWritable())
.runWith(Sink.last(), system);
log.info("Return future on deregisterCluster: {}", clusterId);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Might be more appropriate to use trace or debug level here.

@codyrioux
Copy link
Contributor

codyrioux commented Apr 25, 2022 via email

@Andyz26 Andyz26 merged commit 48db53a into Netflix:master Apr 25, 2022
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