Skip to content

Commit

Permalink
Merge pull request #2476 from murgatroid99/grpc-js_prohibit_od_pick_f…
Browse files Browse the repository at this point in the history
…irst

grpc-js: Disallow pick_first as child of outlier_detection
  • Loading branch information
murgatroid99 authored Jun 27, 2023
2 parents 073caf5 + ed70a0b commit 409418b
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 8 deletions.
2 changes: 1 addition & 1 deletion packages/grpc-js/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@grpc/grpc-js",
"version": "1.8.16",
"version": "1.8.17",
"description": "gRPC Library for Node - pure JS implementation",
"homepage": "https://grpc.io/",
"repository": "https://github.com/grpc/grpc-node/tree/master/packages/grpc-js",
Expand Down
11 changes: 7 additions & 4 deletions packages/grpc-js/src/load-balancer-outlier-detection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ export class OutlierDetectionLoadBalancingConfig implements LoadBalancingConfig
failurePercentageEjection: Partial<FailurePercentageEjectionConfig> | null,
private readonly childPolicy: LoadBalancingConfig[]
) {
if (childPolicy.length > 0 && childPolicy[0].getLoadBalancerName() === 'pick_first') {
throw new Error('outlier_detection LB policy cannot have a pick_first child policy');
}
this.intervalMs = intervalMs ?? 10_000;
this.baseEjectionTimeMs = baseEjectionTimeMs ?? 30_000;
this.maxEjectionTimeMs = maxEjectionTimeMs ?? 300_000;
Expand Down Expand Up @@ -395,8 +398,8 @@ export class OutlierDetectionLoadBalancer implements LoadBalancer {
}

private isCountingEnabled(): boolean {
return this.latestConfig !== null &&
(this.latestConfig.getSuccessRateEjectionConfig() !== null ||
return this.latestConfig !== null &&
(this.latestConfig.getSuccessRateEjectionConfig() !== null ||
this.latestConfig.getFailurePercentageEjectionConfig() !== null);
}

Expand Down Expand Up @@ -496,7 +499,7 @@ export class OutlierDetectionLoadBalancer implements LoadBalancer {
if (addressesWithTargetVolume < failurePercentageConfig.minimum_hosts) {
return;
}

// Step 2
for (const [address, mapEntry] of this.addressMap.entries()) {
// Step 2.i
Expand Down Expand Up @@ -656,4 +659,4 @@ export function setup() {
if (OUTLIER_DETECTION_ENABLED) {
registerLoadBalancerType(TYPE_NAME, OutlierDetectionLoadBalancer, OutlierDetectionLoadBalancingConfig);
}
}
}
4 changes: 2 additions & 2 deletions packages/grpc-js/src/resolver-dns.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ class DnsResolver implements Resolver {
details: `Name resolution failed for target ${uriToString(this.target)}`,
metadata: new Metadata(),
};

const backoffOptions: BackoffOptions = {
initialDelay: channelOptions['grpc.initial_reconnect_backoff_ms'],
maxDelay: channelOptions['grpc.max_reconnect_backoff_ms'],
Expand Down Expand Up @@ -276,7 +276,7 @@ class DnsResolver implements Resolver {
} catch (err) {
this.latestServiceConfigError = {
code: Status.UNAVAILABLE,
details: 'Parsing service config failed',
details: `Parsing service config failed with error ${(err as Error).message}`,
metadata: new Metadata(),
};
}
Expand Down
12 changes: 11 additions & 1 deletion packages/grpc-js/test/test-outlier-detection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,16 @@ describe('Outlier detection config validation', () => {
}, /failure_percentage_ejection\.enforcement_percentage parse error: value out of range for percentage/);
});
});
describe('child_policy', () => {
it('Should reject a pick_first child_policy', () => {
const loadBalancingConfig = {
child_policy: [{pick_first: {}}]
};
assert.throws(() => {
OutlierDetectionLoadBalancingConfig.createFromJson(loadBalancingConfig);
}, /outlier_detection LB policy cannot have a pick_first child policy/);
});
});
});

describe('Outlier detection', () => {
Expand Down Expand Up @@ -533,4 +543,4 @@ describe('Outlier detection', () => {
})
});
});
});
});

0 comments on commit 409418b

Please sign in to comment.