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 layer max cores #1125

Conversation

akim-ruslanov
Copy link
Contributor

@akim-ruslanov akim-ruslanov commented Apr 5, 2022

Link the Issue(s) this Pull Request is related to.
Currently, the SET_ALL_RENDER_LAYER_CORES filter action only affects the layer's min cores. This can be improved since in some cases setting only min cores or max cores is more beneficial.

Summarize your change.
Changes SET_ALL_RENDER_LAYER_CORES to SET_ALL_RENDER_MIN_CORES and SET_ALL_RENDER_MAX_CORES which allows to set only max cores and/or min cores per layer.

@akim-ruslanov
Copy link
Contributor Author

Previous PR was closed due to CLA issues.

Regarding the comments, the minor version was changed in order to represent changes to proto.

Copy link
Collaborator

@DiegoTavares DiegoTavares left a comment

Choose a reason for hiding this comment

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

No comments on the code.

Please add more context to the PR description mentioning the field already existed and is already part of the dispatchQuery, the only missing part is the ability to update it. While reviewing I was questioning if the newly added int_cores_max was already part of the layer table because I don't see changes to the db schema.

@akim-ruslanov
Copy link
Contributor Author

While reviewing I was questioning if the newly added int_cores_max was already part of the layer table because I don't see changes to the db schema.

int_cores_max is already part of the layer table as can be seen in V1__Initial_schema.sql

@akim-ruslanov
Copy link
Contributor Author

Description updated.

@DiegoTavares
Copy link
Collaborator

@bcipriano FYI

Copy link
Collaborator

@bcipriano bcipriano left a comment

Choose a reason for hiding this comment

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

Change LGTM, just one minor comment about the proto file, and please resolve that conflict with VERSION.in (but make sure this PR still increments the minor version after the conflict is resolved).

proto/filter.proto Show resolved Hide resolved
@akim-ruslanov akim-ruslanov force-pushed the add_layer_max_cores_pr branch from 6ba0446 to f9a7200 Compare May 25, 2022 21:19
@akim-ruslanov akim-ruslanov force-pushed the add_layer_max_cores_pr branch from f9a7200 to afb599a Compare May 25, 2022 21:26
@bcipriano bcipriano merged commit 81a272a into AcademySoftwareFoundation:master Jun 4, 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.

3 participants