Copilot commented on code in PR #17248:
URL: https://github.com/apache/pinot/pull/17248#discussion_r2657019812
##########
pinot-controller/src/main/resources/app/utils/PinotMethodUtils.ts:
##########
@@ -182,24 +182,87 @@ 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;
Review Comment:
The inconsistent type checking for boolean values (string 'true' vs boolean
true) suggests the API response format is ambiguous. Consider normalizing this
at the API layer or adding a comment explaining why both checks are necessary.
##########
pinot-controller/src/main/resources/app/utils/PinotMethodUtils.ts:
##########
@@ -182,24 +182,87 @@ 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`, {
Review Comment:
Constructing HTTP URLs directly from hostname and port without validation
could allow Server-Side Request Forgery (SSRF) attacks. Validate that hostname
and port come from trusted sources (e.g., controller-managed instances) and
consider using a relative path through the controller proxy instead of direct
instance HTTP calls.
```suggestion
// Basic validation to ensure hostname and port are well-formed before
issuing the request.
// This helps avoid constructing arbitrary URLs from untrusted input.
if (typeof hostname !== 'string' || hostname.length === 0) {
return false;
}
// Allow only typical hostname characters (letters, digits, dots and
hyphens).
if (!/^[a-zA-Z0-9.-]+$/.test(hostname)) {
return false;
}
const portNumber = typeof port === 'string' ? Number(port) : port;
if (!Number.isInteger(portNumber) || portNumber <= 0 || portNumber >
65535) {
return false;
}
try {
const response = await
baseApi.get(`http://${hostname}:${portNumber}/health`, {
```
##########
pinot-controller/src/main/resources/app/utils/PinotMethodUtils.ts:
##########
@@ -182,24 +182,87 @@ 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) => {
+ if (!record.isAlive) {
+ return { ...record, healthStatus: 'Dead' };
+ }
+
+ // Determine which port to use for health check
+ // For brokers and servers, use adminPort if available, otherwise use
main port
+ const healthPort = record.adminPort && record.adminPort > 0 ?
record.adminPort : record.port;
Review Comment:
The logic for determining which port to use for health checks is embedded in
this function. Consider extracting this into a separate helper function like
`getHealthCheckPort(record)` for better reusability and testability.
##########
pinot-controller/src/main/resources/app/utils/PinotMethodUtils.ts:
##########
@@ -182,24 +182,87 @@ 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) => {
Review Comment:
Health checks are performed sequentially for all instances using
Promise.all, which could cause delays when checking many instances. Consider
implementing concurrent health checks with a configurable concurrency limit or
add result caching to reduce redundant checks on page refreshes.
--
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]
