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

Added changes to integrade cpu AC to ResourceUsageCollector and Emit Stats #1

Closed

Conversation

ajaymovva
Copy link
Owner

Description

[Describe what this change achieves]

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@ajaymovva ajaymovva changed the title Admission control test Added changes to integrade cpu AC to ResourceUsageCollector and Emit Stats Oct 19, 2023
in -> new ConcreteShardRequest<>(requestReader, in),
this::handlePrimaryRequest
);
if(transportPrimaryAction.equals(TransportShardBulkAction.ACTION_NAME + PRIMARY_ACTION_SUFFIX)){

Choose a reason for hiding this comment

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

Lets add a comment why we are doing this

/**
* Map to store the actions based on the type of the transport operation
*/
public class AdmissionControlActionsMap {

Choose a reason for hiding this comment

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

Lets remove this

@@ -60,8 +68,8 @@ public void initialise() {
/**
* Handler to trigger registered admissionController

Choose a reason for hiding this comment

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

Lets add comments for method parameters as well , as action is mainly needed for debugging in this case

/**
*
* @param settings Immutable settings instance
* @param clusterSettings ClusterSettings Instance
* @param clusterService ClusterSettings Instance

Choose a reason for hiding this comment

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

Same comment everywhere - lets update comments to reflect the function parameters

return false;
}

private long getMaxCPULimitForAction(AdmissionControlActionType transportActionType) {

Choose a reason for hiding this comment

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

getCpuRejectionThreshold

*/
@Override
public void writeTo(StreamOutput out) throws IOException {
if (out.getVersion().onOrAfter(Version.CURRENT)) {

Choose a reason for hiding this comment

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

Use same version constant in both in and out

@ajaymovva ajaymovva force-pushed the feature/admission-controller branch 11 times, most recently from b8c2c63 to fa39f9a Compare October 20, 2023 12:34
@ajaymovva ajaymovva deleted the branch feature/admission-controller October 24, 2023 10:05
@ajaymovva ajaymovva closed this Oct 24, 2023
@ajaymovva ajaymovva deleted the admission_control_test branch February 9, 2024 12:08
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