github-actions[bot] commented on code in PR #68449:
URL: https://github.com/apache/doris/pull/68449#discussion_r4091361896
##########
regression-test/suites/show_p0/test_show_create_table_and_views_nereids.groovy:
##########
@@ -54,19 +50,60 @@ suite("test_show_create_table_and_views_nereids", "show") {
String rollupName = "${suiteName}_rollup"
String likeName = "${suiteName}_like"
- def forceReplicaAllocation =
getFeConfig('force_olap_table_replication_allocation').trim()
- def effectiveReplicaNum = 1
- if (!forceReplicaAllocation.isEmpty()) {
- def matcher = forceReplicaAllocation =~
/^tag\.location\.default:\s*(1|3)$/
- assertTrue(matcher.matches(),
- "Unsupported force_olap_table_replication_allocation:
${forceReplicaAllocation}")
- effectiveReplicaNum = matcher.group(1).toInteger()
- } else {
- def forceReplicaNum =
getFeConfig('force_olap_table_replication_num').toInteger()
- if (forceReplicaNum > 0) {
- assertTrue(forceReplicaNum in [1, 3],
- "Unsupported force_olap_table_replication_num:
${forceReplicaNum}")
- effectiveReplicaNum = forceReplicaNum
+ String expectedReplicaAllocation =
getFeConfig('force_olap_table_replication_allocation')?.trim()
+ if (!expectedReplicaAllocation) {
+ def forcedReplicaNum =
getFeConfig('force_olap_table_replication_num').toInteger()
+ expectedReplicaAllocation = "tag.location.default: ${forcedReplicaNum
> 0 ? forcedReplicaNum : 1}"
+ }
+ def replicaAllocationToMap = { String allocation ->
+ allocation.split(',').collectEntries { String entry ->
+ def parts = entry.trim().split(/\s*:\s*/, 2)
+ assertEquals(2, parts.length, "Invalid replica allocation:
${allocation}")
+ [(parts[0]): parts[1].toInteger()]
+ }
+ }
+ String initialCreateTable = null
+ def checkShowCreateTable = { String qualifiedName, String expectedTable ->
+ def result = sql_return_maparray "SHOW CREATE TABLE ${qualifiedName}"
+ assertEquals(1, result.size())
+ assertEquals(expectedTable, result[0]["Table"].toString())
+ def createTable = result[0]["Create Table"].toString()
+ def tablePrefix = "CREATE TABLE `${expectedTable}`"
+ assertTrue(createTable.startsWith(tablePrefix), "SHOW CREATE TABLE
${qualifiedName} returned: ${createTable}")
+ // SHOW CREATE uses double-quoted index comments and escapes an
embedded double quote.
+ [
+ "`user_id` largeint NOT NULL",
+ "`good_id` largeint NOT NULL",
+ "`cost` bigint SUM NULL DEFAULT \"0\"",
+ "INDEX index_user_id (`user_id`) USING INVERTED COMMENT \"test
index comment\"",
+ "INDEX index_good_id (`good_id`) USING INVERTED COMMENT \"test
index\\\" comment\"",
+ "AGGREGATE KEY(`user_id`, `good_id`)",
+ "PARTITION BY RANGE(`good_id`)",
+ 'PARTITION p1 VALUES
[("-170141183460469231731687303715884105728"), ("100"))',
+ 'PARTITION p2 VALUES [("100"), ("200"))',
+ 'PARTITION p3 VALUES [("200"), ("300"))',
+ 'PARTITION p4 VALUES [("300"), ("400"))',
+ 'PARTITION p5 VALUES [("400"), ("500"))',
+ 'PARTITION p6 VALUES [("500"), ("600"))',
+ 'PARTITION p7 VALUES [("600"), (MAXVALUE)))',
+ "DISTRIBUTED BY HASH(`user_id`) BUCKETS 2",
+ "\"storage_format\" = \"V2\""
+ ].each {
+ assertTrue(createTable.contains(it), "SHOW CREATE TABLE
${qualifiedName} should contain ${it}, actual: ${createTable}")
+ }
+ def replicaAllocation = (createTable =~ /"replication_allocation" =
"([^"]+)"/)
+ assertTrue(replicaAllocation.find(), "SHOW CREATE TABLE
${qualifiedName} has no replica allocation: ${createTable}")
Review Comment:
[P1] Keep this assertion cloud-aware
Removing the cloud skip makes this assertion deterministically fail:
`Env.addOlapTablePropertyInfo` emits `file_cache_ttl_seconds` in cloud mode and
appends `replication_allocation` only in the non-cloud branch. Please validate
the cloud property separately (or retain the skip) before dereferencing this
matcher.
##########
regression-test/suites/load_p0/stream_load/test_stream_load_endpoint.groovy:
##########
@@ -55,43 +56,70 @@ suite('test_stream_load_endpoint', 'docker') {
// get fe and be info
def feIp = cluster.getMasterFe().getHttpAddress()[0]
def fePort = cluster.getMasterFe().getHttpAddress()[1]
-
+
def be1Ip = cluster.getBackends().get(0).getHttpAddress()[0]
def be1HttpPort = cluster.getBackends().get(0).getHttpAddress()[1]
def be1HeartbeatPort = cluster.getBackends().get(0).getHeartbeatPort()
-
+
def be2Ip = cluster.getBackends().get(1).getHttpAddress()[0]
def be2HttpPort = cluster.getBackends().get(1).getHttpAddress()[1]
def be2HeartbeatPort = cluster.getBackends().get(1).getHeartbeatPort()
-
+
def be3Ip = cluster.getBackends().get(2).getHttpAddress()[0]
def be3HttpPort = cluster.getBackends().get(2).getHttpAddress()[1]
def be3HeartbeatPort = cluster.getBackends().get(2).getHeartbeatPort()
-
+
log.info("Initial cluster setup - 3 BEs")
log.info("BE1: ${be1Ip}:${be1HeartbeatPort}, BE2:
${be2Ip}:${be2HeartbeatPort}, BE3: ${be3Ip}:${be3HeartbeatPort}")
sql """show backends"""
-
+
// Drop BE1 and BE2
log.info("Dropping BE1 and BE2")
sql """ALTER SYSTEM DROPP BACKEND '${be1Ip}:${be1HeartbeatPort}'"""
sql """ALTER SYSTEM DROPP BACKEND '${be2Ip}:${be2HeartbeatPort}'"""
log.info("Backends after dropping BE1 and BE2: ${sql """show
backends""" }")
-
+
// Add BE1 back with custom endpoints
log.info("Adding BE1 back with custom public and private endpoints")
sql """ALTER SYSTEM ADD BACKEND '${be1Ip}:${be1HeartbeatPort}'
properties('tag.public_endpoint' = '11.10.10.10:8010', 'tag.private_endpoint' =
'10.10.10.9:8020')"""
-
+
// Add BE2 back with different custom endpoints
log.info("Adding BE2 back with different custom endpoints")
sql """ALTER SYSTEM ADD BACKEND '${be2Ip}:${be2HeartbeatPort}'
properties('tag.public_endpoint' = '12.20.20.20:8030', 'tag.private_endpoint' =
'11.20.20.19:8040')"""
-
+
// Modify BE3's endpoints
log.info("Modifying BE3's endpoints")
sql """ALTER SYSTEM MODIFY BACKEND '${be3Ip}:${be3HeartbeatPort}' set
('tag.public_endpoint' = '13.30.30.30:8050', 'tag.private_endpoint' =
'12.30.30.29:8060', 'tag.location' = 'default')"""
-
+
+ // Wait for BE1 and BE2 to become alive
+ log.info("Waiting for all BEs to become alive...")
+ def maxRetries = 60
+ def retryCount = 0
+ def allAlive = false
+ while (retryCount < maxRetries) {
+ def backends = sql """show backends"""
+ def aliveCount = 0
+ backends.each { backend ->
+ if (backend[9] == true || backend[9] == "true") { // Column 9
is the 'Alive' status
+ aliveCount++
+ }
+ }
+ log.info("Retry ${retryCount}: ${aliveCount} BEs are alive out of
${backends.size()}")
+ if (aliveCount >= 3) {
+ allAlive = true
+ log.info("All 3 BEs are now alive")
+ break
+ }
+ sleep(1000)
+ retryCount++
+ }
+
+ if (!allAlive) {
Review Comment:
[P1] Fail when the re-added backends never become ready
This timeout is a failed prerequisite, but the test continues and every
redirect assertion accepts BE3, the backend that was never dropped. Thus
BE1/BE2 can remain dead and the re-add path still passes entirely through BE3.
Please fail/assert here and include the final `SHOW BACKENDS` state in the
diagnostic.
##########
regression-test/suites/show_p0/test_show_processlist.groovy:
##########
@@ -99,7 +99,7 @@ suite("test_show_processlist") {
}
}
- def result1 = connect('root', context.config.jdbcPassword,
context.config.jdbcUrl) {
+ def result1 = connect('root', context.config.getRootPassword(),
context.config.jdbcUrl) {
Review Comment:
[P1] Migrate the earlier root connection too
The nested admin connection at line 89 still calls `connect('root',
context.config.jdbcPassword, ...)`. With a configured non-root regression user,
the new `getRootPassword()` contract intentionally returns the empty root
password, while line 89 sends the other user's password and fails before this
corrected call is reached. Please use `getRootPassword()` for both root
connections.
##########
regression-test/suites/point_query_p0/test_point_query.groovy:
##########
@@ -378,7 +378,7 @@ suite("test_point_query") {
sql """ADMIN SET FRONTEND CONFIG ("enable_lightweight_lookup_request"
= "true")"""
try {
- connect(user, password, prepare_url) {
+ connectToDoris(user, password, prepare_url) {
Review Comment:
[P1] Migrate the two earlier prepared connections
The metadata-change blocks at lines 336 and 366 still call generic
`connect(user, password, prepare_url)`. That URL is manually rebuilt without
client-certificate parameters, so with `enableTLS=true`, `SuiteContext.connect`
returns the augmented URL without opening a connection or invoking either
closure. Please use `connectToDoris` for those two blocks as well.
##########
regression-test/suites/doc/data-operate/update/update-of-aggregate-model.md.groovy:
##########
@@ -36,7 +36,13 @@
suite("docs/data-operate/update/update-of-aggregate-model.md", "p0,nonConcurrent
(1 , 100 , 'Pending' );
"""
- cmd """curl --location-trusted -u
${context.config.jdbcUser}:${context.config.jdbcPassword} -H
"column_separator:," -H "columns:order_id,order_status" -T
${context.file.parent}/update.csv
http://${context.config.feHttpAddress}/api/${curDbName}/order_tbl/_stream_load""";
+ boolean enableTls =
context.config.otherConfigs.get("enableTLS")?.toString()?.equalsIgnoreCase("true")
?: false
+ String protocol = enableTls ? "https" : "http"
+ String tlsOptions = ""
+ if (enableTls) {
+ tlsOptions = " --cert
${context.config.otherConfigs.get('trustCert')} --key
${context.config.otherConfigs.get('trustCAKey')} --cacert
${context.config.otherConfigs.get('trustCACert')}"
Review Comment:
[P1] Reuse the shell-quoting TLS helper
These certificate paths are interpolated unquoted into a command executed by
`/bin/bash -c`, so a valid path with spaces is split (and shell metacharacters
are interpreted). This PR adds `getDorisCurlTlsOptions()` specifically to quote
these values safely. Please use it here and in
`partition_p1/auto_partition/sql/multi_thread_load.groovy:133`, which repeats
the same construction.
##########
regression-test/suites/mysql_ssl_p0/test_mysql_connection.groovy:
##########
@@ -14,50 +14,31 @@
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
-import org.apache.doris.regression.Config
+import org.apache.doris.regression.util.MySqlClient
suite("test_mysql_connection") { suite ->
- // NOTE: this suite need you install mysql client 5.7 + to support
--ssl-mode parameter
-
- def executeMySQLCommand = { String command ->
- def cmds = ["/bin/bash", "-c", command]
- logger.info("Execute: ${cmds}".toString())
- Process p = cmds.execute()
-
- def errMsg = new StringBuilder()
- def msg = new StringBuilder()
- p.waitForProcessOutput(msg, errMsg)
-
- assert errMsg.length() == 0: "error occurred!" + errMsg
- assert msg.toString().contains("version"): "error occurred!" + errMsg
- assert p.exitValue() == 0
+ // NOTE: this suite needs mysql client 5.7+ to support --ssl-mode.
+ URI endpoint = new URI(context.config.jdbcUrl.substring('jdbc:'.length()))
Review Comment:
[P1] Move URL parsing behind the TLS skip
In an `enableTLS` run, `context.config.jdbcUrl` already contains the raw
key/trust-store paths added by `Config.createDefaultDb`. A supported path
containing a space makes `new URI(...)` throw before the guard at line 31, even
though this suite intentionally does nothing in that mode. Please only parse
the endpoint inside the non-TLS branch (or parse just the JDBC authority
without its query).
##########
regression-test/framework/src/main/groovy/org/apache/doris/regression/util/WarmupMetricsUtils.groovy:
##########
@@ -115,6 +115,85 @@ class WarmupMetricsUtils {
Thread.sleep(waitMs)
}
+ private static String backendIp(Object backend) {
+ return backend instanceof Map ? backend.ip.toString() :
backend[1].toString()
+ }
+
+ private static String backendHttpPort(Object backend) {
+ return backend instanceof Map ? backend.httpPort.toString() :
backend[4].toString()
+ }
+
+ private static String backendBrpcPort(Object backend) {
+ return backend instanceof Map ? backend.brpcPort.toString() :
backend[5].toString()
+ }
+
+ static Map getBackendMetricValues(Collection backends, String metricName) {
+ Map values = [:]
+ for (be in backends) {
+ String key = "${backendIp(be)}:${backendBrpcPort(be)}"
+ values[key] = getBrpcMetric(backendIp(be), backendBrpcPort(be),
metricName)
Review Comment:
[P1] Make the expanded metrics helper TLS-aware
The new backend aggregation path delegates to `getBrpcMetric`, which still
opens a hard-coded `http://` URL; `clearFileCache` and `getPrometheusMetrics`
in this utility do the same. Strict-TLS deployments expose these endpoints over
TLS (and BE explicitly installs SSL options on its BRPC server), so the many
newly migrated suites fail at these helpers. Please centralize these opens
through `Http.openConnection` (including cache clear and Prometheus reads)
before expanding their use.
--
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]
