Copilot commented on code in PR #12621: URL: https://github.com/apache/gravitino/pull/12621#discussion_r3859403235
########## conf/gravitino.conf.template: ########## @@ -43,6 +43,17 @@ gravitino.server.webserver.threadPoolWorkQueueSize = 100 gravitino.server.webserver.requestHeaderSize = 131072 # The response header size of the built-in web server gravitino.server.webserver.responseHeaderSize = 131072 +# UI inactivity timeout in milliseconds. The UI environment variable +# NEXT_PUBLIC_IDLE_TIMEOUT_MS is used when this config is not set. +# gravitino.server.webserver.sessionIdleTimeoutMs = 900000 +# Maximum UI session duration in milliseconds, regardless of user activity. The UI environment +# variable NEXT_PUBLIC_MAX_SESSION_DURATION_MS is used when this config is not set. +# gravitino.server.webserver.sessionMaxDurationMs = 18000000 +# UI warning countdown duration in milliseconds before the inactivity timeout. The UI environment +# variable NEXT_PUBLIC_IDLE_WARNING_LEAD_MS is used when this config is not set. +# gravitino.server.webserver.sessionIdleWarningLeadMs = 60000 +# multiple visibleConfigs are spitted by comma. +# gravitino.server.visibleConfigs=gravitino.server.webserver.sessionIdleTimeoutMs,gravitino.server.webserver.sessionMaxDurationMs,gravitino.server.webserver.sessionIdleWarningLeadMs Review Comment: Typo/grammar in the visibleConfigs comment: “spitted” is incorrect English, and the sentence should describe comma-separated values clearly. Also keep spacing around '=' consistent with the rest of the template for readability. ########## conf/gravitino.conf.template: ########## @@ -88,8 +99,7 @@ gravitino.fetchFile.blockUnsafeRemoteUri = true # THE CONFIGURATION FOR authorization # Whether Gravitino enable authorization or not gravitino.authorization.enable = false -# The admins of Gravitino service, multiple admins are spitted by comma. -gravitino.server.visibleConfigs=gravitino.authorization.serviceAdmins +# The admins of Gravitino service Review Comment: The updated comment for `gravitino.authorization.serviceAdmins` no longer mentions that multiple admins can be specified (comma-separated). This is a doc regression compared to the previous wording and can confuse operators editing the template. ########## web-v2/web/src/lib/provider/sessionTimeoutConfig.js: ########## @@ -0,0 +1,62 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +const SESSION_IDLE_TIMEOUT_KEY = 'gravitino.server.webserver.sessionIdleTimeoutMs' + +const SESSION_MAX_DURATION_KEY = 'gravitino.server.webserver.sessionMaxDurationMs' + +const SESSION_IDLE_WARNING_LEAD_KEY = 'gravitino.server.webserver.sessionIdleWarningLeadMs' + +function resolveSessionDuration(serverValue, envValue, defaultValue) { + const serverDuration = Number(serverValue) + if (Number.isFinite(serverDuration) && serverDuration > 0) { + return serverDuration + } + + const envDuration = Number(envValue) + + return Number.isFinite(envDuration) && envDuration > 0 ? envDuration : defaultValue +} + +/** + * Resolves all UI session timeout settings. + * + * @param {Object} systemConfig values returned by the /configs endpoint + * @param {Object} defaults built-in fallback values + * @returns {{idleTimeoutMs: number, warningLeadMs: number, maxSessionDurationMs: number}} + */ +export function resolveSessionTimeouts(systemConfig, defaults) { + return { + idleTimeoutMs: resolveSessionDuration( + systemConfig?.[SESSION_IDLE_TIMEOUT_KEY], + process.env.NEXT_PUBLIC_IDLE_TIMEOUT_MS, Review Comment: This new precedence logic (server config → NEXT_PUBLIC_* env → defaults) is core behavior but currently has no unit tests. Please add tests that cover: (1) server value wins over env/default, (2) env wins over default when server is absent/invalid, (3) invalid/zero/negative values fall back as expected. -- 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]
