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

Replicas limit #465

Merged
merged 5 commits into from
May 8, 2023
Merged

Replicas limit #465

merged 5 commits into from
May 8, 2023

Conversation

wanliAlex
Copy link
Collaborator

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    we add a new environmental variable, "MARQO_MAX_NUMBER_OF_REPLICAS" to control the maximum number of replicas that can be set when creating an index.

  • What is the current behavior? (You can also link to an open issue here)
    no limit for number of replicas

  • What is the new behavior (if this is a feature change)?
    we have this limit now

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
    no

  • Have unit tests been run against this PR? (Has there also been any additional testing?)
    running

  • Related Python client changes (link commit/PR here)
    no

  • Related documentation changes (link commit/PR here)
    will do

  • Other information:
    no

  • Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added / updated (for bug fixes / features)

@wanliAlex wanliAlex requested a review from pandu-k May 5, 2023 05:51
Copy link
Collaborator

@pandu-k pandu-k left a comment

Choose a reason for hiding this comment

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

added comment

@wanliAlex wanliAlex temporarily deployed to marqo-test-suite May 8, 2023 03:33 — with GitHub Actions Inactive
Copy link
Contributor

@vicilliar vicilliar left a comment

Choose a reason for hiding this comment

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

looks good! I just mentioned 1 more possible test you could add, but it shouldn't be a big deal

@@ -128,7 +128,7 @@
NsFields.hnsw_ef_construction: {
"type": "integer",
"minimum": 1,
"maximum": int(read_env_vars_and_defaults(EnvVars.MARQO_EF_CONSTRUCTION_MAX_VALUE)),
"maximum": read_env_vars_and_defaults_ints(EnvVars.MARQO_EF_CONSTRUCTION_MAX_VALUE),
Copy link
Contributor

Choose a reason for hiding this comment

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

may be the wrong PR to comment this on, but maybe we should rename MARQO_EF_CONSTRUCTION_MAX_VALUE to MARQO_MAX_EF_CONSTRUCTION_VALUE, because if you observe the other env vars, the word MAX always comes first. It may confuse users if we keep EF like that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

good thinking, to keep things consistent, but that would technically be a breaking change

Copy link
Contributor

@vicilliar vicilliar left a comment

Choose a reason for hiding this comment

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

looks good! I just mentioned 1 more possible test you could add, but it shouldn't be a big deal

@wanliAlex wanliAlex temporarily deployed to marqo-test-suite May 8, 2023 06:49 — with GitHub Actions Inactive
@wanliAlex wanliAlex merged commit 15f3093 into mainline May 8, 2023
@wanliAlex wanliAlex deleted the replicas-limit branch May 8, 2023 08:25
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.

3 participants