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

[serving] Adds mutliple node cluster configuration support #2190

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

frankfliu
Copy link
Contributor

Description

Brief description of what this PR is about

  • If this change is a backward incompatible change, why must this change be made?
  • Interesting edge cases to note here

@frankfliu frankfliu requested review from zachgk and a team as code owners July 17, 2024 23:18
binding = prop.getProperty(INFERENCE_ADDRESS, "http://127.0.0.1:8080");
} else {
binding = prop.getProperty(CLUSTER_ADDRESS, "http://127.0.0.1:8888");
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't correct for ConnectorType.BOTH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change to switch case to make it more readable

private String error;

private ClusterConfig() {
clusterSize = Integer.parseInt(Utils.getenv("DJL_CLUSTER_SIZE", "1"));
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be slightly nicer to configure this using the config manager rather than just environment variables

Copy link
Contributor Author

@frankfliu frankfliu Jul 17, 2024

Choose a reason for hiding this comment

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

  1. This is configured by LWS, and it only support env var so far
  2. There are other code (PyEnv) is also just use this env var

@frankfliu frankfliu force-pushed the cluster branch 3 times, most recently from f3fb43b to 7a4fd07 Compare July 18, 2024 01:07
@@ -1,5 +1,6 @@
inference_address=http://0.0.0.0:8080
management_address=http://0.0.0.0:8080
cluster_address=http://0.0.0.0:8888
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: any reason for choosing 8888? Should it be a closer port like 8081?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

User may choose management port as 8081

@frankfliu frankfliu merged commit aab6756 into deepjavalibrary:master Jul 18, 2024
8 checks passed
@frankfliu frankfliu deleted the cluster branch July 18, 2024 22:18
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.

4 participants