Copilot commented on code in PR #17450:
URL: https://github.com/apache/pinot/pull/17450#discussion_r2657011993
##########
pinot-controller/src/main/resources/app/utils/PinotMethodUtils.ts:
##########
@@ -184,24 +186,82 @@ const getAllInstances = () => {
});
};
+// This method is used to check the health endpoint of an instance
+// API: http://{hostname}:{port}/health
+// Expected Output: 'OK' or error
+const checkInstanceHealth = async (hostname, port) => {
+ try {
+ const response = await baseApi.get(`http://${hostname}:${port}/health`, {
+ timeout: 5000 // 5 second timeout
+ });
+ return response.data === 'OK' || response.status === 200;
+ } catch (error) {
+ return false;
+ }
+};
+
// This method is used to display instance data on cluster manager home page
// API: /instances/:instanceName
// Expected Output: {columns: [], records: []}
const getInstanceData = (instances, liveInstanceArr) => {
const promiseArr = [...instances.map((inst) => getInstance(inst))];
- return Promise.all(promiseArr).then((result) => {
+ return Promise.all(promiseArr).then(async (result) => {
+ // First, get all instance configs
+ const instanceRecords = result.map(({ data }) => {
+ const isAlive = liveInstanceArr.indexOf(data.instanceName) > -1;
+ const isEnabled = data.enabled;
+ const queriesDisabled = data.queriesDisabled === 'true' ||
data.queriesDisabled === true;
+ const shutdownInProgress = data.shutdownInProgress === true;
+
+ return {
+ instanceName: data.instanceName,
+ enabled: data.enabled,
+ hostName: data.hostName,
+ port: data.port,
+ adminPort: data.adminPort,
+ isAlive,
+ isEnabled,
+ queriesDisabled,
+ shutdownInProgress
+ };
+ });
+
+ // Then check health endpoints for alive instances
+ const healthCheckPromises = instanceRecords.map(async (record) => {
+
+ let status: InstanceStatusCell = {value: InstanceStatus.DEAD, tooltip:
'Instance is not running'};
+
+ if (!record.isAlive) {
+ return { ...record, healthStatus: status };
+ }
+
+ if (record.isAlive) {
+ if (record.shutdownInProgress) {
+ status = {value: InstanceStatus.UNHEALTHY, tooltip: 'Instance is
running but is starting up or shutting down'};
+ } else if (!record.isEnabled) {
+ status = {value: InstanceStatus.INSTANCE_DISABLED, tooltip:
'Instance has been disabled in helix'};
+ } else if (record.queriesDisabled) {
+ status = {value: InstanceStatus.QUERIES_DISABLED, tooltip: 'Instance
running but has queries disabled'};
+ } else {
+ status = {value: InstanceStatus.HEALTHY, tooltip: 'Instance is
healthy and ready to serve requests'};
+ }
Review Comment:
This condition is redundant since line 235-237 already handles the
!record.isAlive case with an early return. The outer if check on line 239 will
always be true when reached.
```suggestion
// record.isAlive is true here because of the early return above
if (record.shutdownInProgress) {
status = {value: InstanceStatus.UNHEALTHY, tooltip: 'Instance is
running but is starting up or shutting down'};
} else if (!record.isEnabled) {
status = {value: InstanceStatus.INSTANCE_DISABLED, tooltip:
'Instance has been disabled in helix'};
} else if (record.queriesDisabled) {
status = {value: InstanceStatus.QUERIES_DISABLED, tooltip: 'Instance
running but has queries disabled'};
} else {
status = {value: InstanceStatus.HEALTHY, tooltip: 'Instance is
healthy and ready to serve requests'};
```
##########
pinot-controller/src/main/resources/app/utils/PinotMethodUtils.ts:
##########
@@ -184,24 +186,82 @@ const getAllInstances = () => {
});
};
+// This method is used to check the health endpoint of an instance
+// API: http://{hostname}:{port}/health
+// Expected Output: 'OK' or error
+const checkInstanceHealth = async (hostname, port) => {
+ try {
+ const response = await baseApi.get(`http://${hostname}:${port}/health`, {
+ timeout: 5000 // 5 second timeout
+ });
+ return response.data === 'OK' || response.status === 200;
+ } catch (error) {
+ return false;
+ }
+};
+
Review Comment:
The checkInstanceHealth function is defined but never used. According to the
PR description, the `/health` endpoint calls were removed because users
shouldn't have direct server access. This unused function should be removed to
avoid confusion.
```suggestion
```
##########
pinot-controller/src/main/resources/app/components/Table.tsx:
##########
@@ -398,7 +398,16 @@ export default function CustomizedTables({
/>
);
}
- if (str.toLowerCase() === 'consuming' || str.toLocaleLowerCase() ===
"partial" || str.toLocaleLowerCase() === "updating" ) {
+ if (str.toLowerCase() === 'consuming' || str.toLocaleLowerCase() ===
"partial" || str.toLocaleLowerCase() === "updating") {
+ return (
+ <StyledChip
+ label={str}
+ className={classes.cellStatusConsuming}
+ variant="outlined"
+ />
+ );
+ }
+ if (str.toLowerCase() === 'disabled' || str.toLocaleLowerCase() ===
"queries disabled") {
Review Comment:
Inconsistent use of toLowerCase() and toLocaleLowerCase() methods, and mixed
quote styles (single vs double quotes). Use toLowerCase() consistently and
maintain consistent quote style throughout the file.
##########
pinot-controller/src/main/resources/app/components/Table.tsx:
##########
@@ -389,7 +389,7 @@ export default function CustomizedTables({
/>
);
}
- if (str.toLocaleLowerCase() === 'bad' || str.toLowerCase() === 'offline'
|| str.toLowerCase() === 'dead' || str.toLowerCase() === 'false') {
+ if (str.toLocaleLowerCase() === 'bad' || str.toLocaleLowerCase() ===
'unhealthy' || str.toLowerCase() === 'offline' || str.toLowerCase() === 'dead'
|| str.toLowerCase() === 'false') {
Review Comment:
Inconsistent use of toLowerCase() and toLocaleLowerCase() methods. The code
uses toLocaleLowerCase() for the first two checks but toLowerCase() for the
rest. Use toLowerCase() consistently unless locale-specific behavior is
required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
