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

Docs: Default Counter Capacity as 1000 #3637

Merged
merged 6 commits into from
Feb 13, 2024

Conversation

Kalaiselvi84
Copy link
Contributor

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug
/kind cleanup
/kind documentation
/kind feature
/kind hotfix
/kind release

What this PR does / Why we need it:

Which issue(s) this PR fixes:

Closes #3579

Special notes for your reviewer:

@github-actions github-actions bot added kind/cleanup Refactoring code, fixing up documentation, etc size/S labels Feb 9, 2024
@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 3bf3f90c-5adc-4780-ad0b-db6eadb274b8

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

@@ -264,6 +264,8 @@ to the capacity of that Counter or List.
All functions will return an error if the specified `key` is not predefined in the
[`GameServer.Spec.Counters`][gameserverspec] resource configuration.

**Note:** For Counters, the default settings for both capacity and count are preset to 1000. It is recommended to avoid configuring the capacity or count to max(int64), as doing so could cause problems with [JSON Patch operations](https://github.com/googleforgames/agones/issues/3636).
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've updated the yaml and .md files to show the default setting of 1000 for counters. Can you please check if these updates are ok or if this Note section in the Agones Game Server Client SDKs documentation covers everything needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Only the capacity defaults to 1000, the count defaults to 0.

@agones-bot
Copy link
Collaborator

Build Failed 😱

Build Id: 93dbae7c-236a-4193-af06-6c932d8b9227

To get permission to view the Cloud Build view, join the agones-discuss Google Group.

examples/counterfleetautoscaler.yaml Outdated Show resolved Hide resolved
examples/gameserver.yaml Outdated Show resolved Hide resolved
examples/gameserverallocation.yaml Outdated Show resolved Hide resolved
pkg/apis/allocation/v1/gameserverallocation.go Outdated Show resolved Hide resolved
site/content/en/docs/Reference/fleet.md Outdated Show resolved Hide resolved
site/content/en/docs/Reference/fleetautoscaler.md Outdated Show resolved Hide resolved
sdks/go/alpha.go Outdated Show resolved Hide resolved
@@ -93,9 +93,9 @@ spec:
# counters: # counters are int64 counters that can be incremented and decremented by set amounts. Keys must be declared at GameServer creation time.
# games: # arbitrary key.
# count: 1 # initial value.
# capacity: 100 # (Optional) Maximum value for the counter. 0 is max(int64).
# capacity: 100 # (Optional) Defaults to 1000 and setting capacity to max(int64) may lead to issues and is not recommended.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# capacity: 100 # (Optional) Defaults to 1000 and setting capacity to max(int64) may lead to issues and is not recommended.
# capacity: 100 # (Optional) Defaults to 1000 and setting capacity to max(int64) may lead to issues and is not recommended [#3636](https://github.com/googleforgames/agones/issues/3636)

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should link back to the GitHub issue so that people can reference the "why". You may need to run make site-server to make sure the link functions correctly on the Agones website https://agones.dev/site/docs/reference/gameserver/.

@Kalaiselvi84
Copy link
Contributor Author

I have updated the gameserver.md and gameserver.yaml, and have reverted the changes in the suggested files. I am working on the hyperlink within the YAML content that is in the Markdown file but am not 100% sure that it is possible.

@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: 0ed24df2-1812-45d8-ab00-0867f4e96d39

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3637/head:pr_3637 && git checkout pr_3637
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.39.0-dev-c129094-amd64

Copy link
Collaborator

@igooch igooch left a comment

Choose a reason for hiding this comment

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

One comment, otherwise looks good!

@@ -264,6 +264,8 @@ to the capacity of that Counter or List.
All functions will return an error if the specified `key` is not predefined in the
[`GameServer.Spec.Counters`][gameserverspec] resource configuration.

**Note:** For Counters, the default settings for both capacity and count are preset to 1000. It is recommended to avoid configuring the capacity or count to max(int64), as doing so could cause problems with [JSON Patch operations](https://github.com/googleforgames/agones/issues/3636).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only the capacity defaults to 1000, the count defaults to 0.

Copy link
Collaborator

@igooch igooch left a comment

Choose a reason for hiding this comment

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

LGTM

@igooch igooch enabled auto-merge (squash) February 12, 2024 22:30
@agones-bot
Copy link
Collaborator

Build Succeeded 👏

Build Id: fe5ad1dc-20e1-4547-8123-5fec8439ac34

The following development artifacts have been built, and will exist for the next 30 days:

A preview of the website (the last 30 builds are retained):

To install this version:

  • git fetch https://github.com/googleforgames/agones.git pull/3637/head:pr_3637 && git checkout pr_3637
  • helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.39.0-dev-0dc77c3-amd64

@igooch igooch merged commit db90b21 into googleforgames:main Feb 13, 2024
4 checks passed
@Kalaiselvi84 Kalaiselvi84 deleted the counter-capacity branch March 15, 2024 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Refactoring code, fixing up documentation, etc size/S size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Counters: Default capacity should be 0 (max), not 1000
3 participants