Repository navigation
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19237 +/- ##
============================================
+ Coverage 48.89% 48.94% +0.05%
Complexity 689 689
============================================
Files 5588 5589 +1
Lines 413444 413897 +453
Branches 54537 54601 +64
============================================
+ Hits 202154 202586 +432
- Misses 211290 211311 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
JackieTien97
left a comment
There was a problem hiding this comment.
Thanks for the PR. I built this head (f53ab9a) as a 1C1D distribution and drove it with a small client program that uses the new ITableSession.insert(Tablet, String). I also ran the PR client against a server built from an older master (2075032) to check mixed versions.
What works
- An explicit database receives the rows, and
nullfalls back to the session database. The session's current database is unchanged afterwards, including afterUSE. - Auto-created tables land in the explicit database.
- Privileges are checked against the explicit (lower-case) database. A user with INSERT only on
db1gets 803 fordb2, and no table is created there.information_schemagives 701 and__auditgives 803. TableSessionPoolworks, and the pooled session's database stays as configured.- Both retry paths reuse the request, so the database survives redirection and broken-connection retries.
- CI: all Java UT/IT jobs pass. The new IT ran in Table 1C1D (31/31) and in 1C3D.
Blocking (details inline)
- A new client talking to an older server silently writes into the wrong database. The old server drops unknown field 14. So
insert(tablet, "cdb2")on a session whose database iscdb1returns success, the rows land incdb1, and a missing table is auto-created incdb1. The description mentions this fallback, but the client neither detects nor reports it. - An upper-case tree prefix gets past the tree-model check, and the rows are written into the tree database.
isTableModelDatabaseis case-sensitive and runs before lower-casing. Take tree databaseroot.treedband table databasetreedb, both present.insert(tablet, "ROOT.treedb")returns success but creates aligned devicesroot.treedb.tt.<tag>in the tree database, and neither model can read the rows. This also works for a user who has only tableINSERT ON DATABASE treedband who gets 803 for a tree insert intoroot.treedb. If there is no matching table database, the same input fails with 500/301 errors that leak internal exception names.
Should fix
-
Database names are not validated.
""gives 301, and" "and"a.b"give 500. All three log a WARN with a full stack trace on every call. The two reused messages are also wrong for this API, in both en and zh. -
ISession.openWithTableModelDatabaseCachewidens the public API for an internal caller. The new abstract methods onISession/ITableSessionalso break third-party implementations. -
Route-cache keys are not normalized, and entries are created before the server validates the name.
-
Tests are missing for:
- rejecting tree-prefixed names, in both lower and upper case
- the grouped multi-device path with an explicit database
Also, the
""assertion doesn't check the status code.
PR description
The Validation section says "Per repository constraints, no build or test execution was performed". No such constraint exists in CONTRIBUTING.md, AGENTS.md or the PR template. In fact, AGENTS.md asks for a full-reactor test-compile in both locales for cross-module and i18n changes. Some intermediate commits did not compile:
- 7ab0db7 is missing the
IDeviceIDimport, fixed in e8a15c6. - The IT's
leaders::addAllonly compiles after f53ab9a.
The head is green, so please just correct that sentence.
CI notes
- The failing Python job is the pandas
OutOfBoundsDatetimeissue fixed on master by #19248, so a rebase should clear it. - The Sonar gate fails on Reliability issues in files this PR doesn't touch (
InsertValues,TierManager,UDFTable).
| 11: optional bool isCompressed | ||
| 12: optional list<byte> encodingTypes | ||
| 13: optional byte compressType | ||
| 14: optional string database |
There was a problem hiding this comment.
Blocking: an older server ignores this field and writes into the session database.
Thrift servers skip unknown optional fields, so a server without this change never sees database and falls back to the session's current database. I ran the PR client against a server built from 2075032:
// session opened with database("cdb1"); cdb1.t1 and cdb2.t1 both exist
session.insert(tablet("t1", 3 rows), "cdb2"); // returns OK
session.insert(tablet("only_in_cdb2", 2 rows), "cdb2"); // returns OKResult: cdb1.t1 gets the 3 rows, cdb2.t1 stays at 0, and table only_in_cdb2 is auto-created in cdb1. With no session database the call fails with 701, but whenever a session database is set the misrouting is silent.
The description acknowledges this fallback ("an older server can ignore the unknown optional field and fall back to its session database"). Documenting it isn't enough, though: the call reports success while the data lands in a different database, and the caller has no way to detect that. A rolling upgrade is a realistic way to hit this, since a redirect can send the request to a node that hasn't been upgraded yet.
Suggestion: in openSession, have the server advertise a capability in TSOpenSessionResp.configuration, next to the existing time-precision entry. The client records it per SessionConnection, because redirect connections are opened separately. When database != null and the connection lacks the capability, the client throws before it sends the request.
| throw new IllegalArgumentException( | ||
| DataNodeQueryMessages.DATABASE_IS_NOT_SPECIFIED_FOR_INSERT); | ||
| } | ||
| if (!PathUtils.isTableModelDatabase(insertTabletReq.getDatabase())) { |
There was a problem hiding this comment.
Blocking: the tree-model check is case-sensitive and runs before lower-casing, so ROOT.xxx gets through and the rows are written into the tree database.
PathUtils.isTableModelDatabase is just !startsWith("root."), so "ROOT.treedb" and "Root.treedb" pass. The name is then lower-cased to root.treedb. Schema validation and the privilege check resolve the table through unQualifyDatabaseName(...), which gives table treedb.tt, while schema/data partitioning and device creation use root.treedb. The other entry points don't have this gap: openSession lower-cases before IClientSession.setDatabaseName checks the name, and SQL rejects a "ROOT.xxx".t prefix with 701 (The tree model database shall not be specified in table model.).
Repro on this head. Tree database root.treedb has device d1, and table database treedb has table tt(tag1 TAG, m1 FIELD):
session.insert(tablet("tt", tags d0, d1; 2 rows), "ROOT.treedb"); // OK
session.insert(tablet("tt", 1 row), "Root.treedb"); // OK
session.insert(tablet("tt", 1 row), "treedb"); // OK (control)| before | after | |
|---|---|---|
table SELECT count(*) FROM treedb.tt |
0 | 1 (only the control row) |
tree COUNT DEVICES root.treedb.** |
1 | 3: new aligned devices root.treedb.tt.d0 and root.treedb.tt.d1 |
tree COUNT TIMESERIES root.treedb.** |
1 | 1 |
The rows are written: after FLUSH they are in a TsFile under sequence/root.treedb/.... Neither model can read them, though. Tree SELECT * FROM root.treedb.tt.d0 returns 0 rows and SHOW TIMESERIES root.treedb.tt.** is empty.
This also gets around the tree privilege. Take a user that has only GRANT INSERT ON DATABASE treedb:
- tree
INSERT INTO root.treedb.d9(time, s1) VALUES (1, 1)→ 803, needsWRITE_DATA on [root.treedb.d9.s1] insert(tablet, "root.treedb")→ 701insert(tablet, "ROOT.treedb")→ OK, and deviceroot.treedb.tt.pv2is created in the tree database
If there is no matching table database, the same input surfaces internal errors instead of a clean 701:
"ROOT.db1"→ 500Create SchemaPartition failed because the database: root.db1 is not exists"ROOT.db1"with a table that doesn't exist yet → 301org.apache.iotdb.confignode.exception.DatabaseNotExistsException: Database: root.db1 doesn't exist."ROOT.treedb"before table databasetreedbexisted → 500Unknown database treedb
Also, the reused DATABASE_NAME_IS_TREE_MODEL_DATABASE ends with "...which is not allowed to set in the client session." (zh: "...不允许设置到客户端会话中。"). This API deliberately leaves the session untouched. THE_TREE_MODEL_DATABASE_SHALL_NOT_BE_SPECIFIED, which the SQL path uses for the same case, fits better.
Suggested shape:
final String database = insertTabletReq.getDatabase().toLowerCase(Locale.ENGLISH);
if (!PathUtils.isTableModelDatabase(database)) {
throw new SemanticException(
DataNodeQueryMessages.THE_TREE_MODEL_DATABASE_SHALL_NOT_BE_SPECIFIED);
}
TableConfigTaskVisitor.validateDatabaseName(database); // see the comment on the empty-name check
insertStatement.setDatabaseName(database);| insertStatement.setColumnCategories(columnCategories); | ||
| if (insertTabletReq.isSetDatabase()) { | ||
| if (insertTabletReq.getDatabase().isEmpty()) { | ||
| throw new IllegalArgumentException( |
There was a problem hiding this comment.
Malformed names come back with inconsistent status codes:
database |
result |
|---|---|
"" |
301 [EXECUTE_STATEMENT_ERROR(301)] Exception occurred: insertTablet failed. database is not specified for insert: |
" " |
500 Unknown database |
"a.b" |
500 Unknown database a.b |
null with no session database |
701 database is not specified |
The DataNode also logs each 301/500 response as a WARN with a full stack trace, on every call. For "" the cause is that ErrorHandlingUtils.onQueryException doesn't recognize IllegalArgumentException, so the exception falls through to onNpeOrUnexpectedException (DN log: StatementGenerator.java:394). In addition, DATABASE_IS_NOT_SPECIFIED_FOR_INSERT is a prefix that is supposed to be followed by the statement, as in the relational Analyzer. Used here, the message ends in a dangling colon (zh: "未指定插入操作的数据库:").
CREATE DATABASE already validates names with TableConfigTaskVisitor.validateDatabaseName. It rejects "", blanks, ., characters outside DATABASE_PATTERN, and names longer than 64. It throws a SemanticException that wraps IllegalPathException, which comes back as 509 and is logged at INFO without a stack trace. Calling it after lower-casing (see the comment above) gives every malformed name a clean user error.
| INodeSupplier nodeSupplier) | ||
| throws IoTDBConnectionException; | ||
|
|
||
| void openWithTableModelDatabaseCache( |
There was a problem hiding this comment.
This adds an internal cache-sharing hook to ISession, a public interface that users program against, and its nested-map parameter exposes the cache layout. The only callers are in SessionPool. The method has to be on the interface only because getSession() declares ISession session = queue.poll();. Opening the new connection through a local Session would avoid that:
Session newSession = constructNewSession();
newSession.openWithTableModelDatabaseCache(...);
session = newSession;tryConstructNewSession() already uses a Session.
Separately, the new abstract methods here and the new ITableSession.insert(Tablet, String) break every third-party implementation of these interfaces at compile time. ISession already uses default methods for this kind of evolution (e.g. isEnableCacheLeader). insert(Tablet, String) could be a default method that delegates to insert(tablet) when database == null and throws UnsupportedOperationException otherwise.
| } | ||
|
|
||
| private String resolveTableModelRouteDatabase(String database) { | ||
| String routeDatabase = database == null ? this.database : database; |
There was a problem hiding this comment.
Nit: the route-cache key is the raw string, while the server lower-cases it, so "DB1" and "db1" get separate route maps for the same database. getTableModelDatabaseRoutes also calls computeIfAbsent before the server has validated the name. Every invalid name ("", "ROOT.x", typos) therefore leaves an empty entry in a map that a SessionPool shares across sessions. Lower-casing here, as the server does, and only creating the entry when a redirect is actually recorded would avoid both problems.
| } | ||
| } | ||
|
|
||
| private static void setRelationalRequestDatabase(TSInsertTabletReq request, String database) { |
There was a problem hiding this comment.
Nit: the null check is redundant. The generated setDatabase(null) leaves the field unset, because isSetDatabase() is database != null. More importantly, the database is attached at three separate call sites, each right after setWriteToTable(true), so a future send path can easily forget it. A single builder for relational tablet requests, taking the tablet and the database, would keep this in one place. That builder is also the natural home for the server-capability check suggested on client.thrift.
| assertEquals("db1", dataSet.next().getField(0).getStringValue()); | ||
| } | ||
|
|
||
| assertThrows(StatementExecutionException.class, () -> session.insert(tablet, "")); |
There was a problem hiding this comment.
Please assert the status code here, not just the exception type. "" currently returns 301, the generic failure code, so this assertion would still pass if the call failed for an unrelated reason.
Missing cases:
- Tree-prefixed names. Try
"root.xxx","ROOT.xxx"and"Root.xxx"while both tree databaseroot.xxxand table databasexxxexist. Assert the error code, and assert that no device appears underroot.xxx.**(see the comment onStatementGenerator). - The grouped path with an explicit database. No test runs
insertRelationalTabletByGroupwith a database set. The new unit test uses a single device. In the IT, each explicit-database insert starts with an empty route map for that database, so it always goes to the default connection. ExtendingSessionCacheLeaderTestwith a multi-device tablet whose devices have different cached leaders, written to an explicit database that differs from the session's, would cover the grouping, the redirect bookkeeping and the per-group requests carrying the database.




Motivation
Table session Tablet insertion currently resolves the target database from the database configured when the session was opened or from the session's current database. A session that needs to write the same Tablet operation to multiple databases must execute
USE, which changes session state and is inconvenient for mixed-database workloads.This change adds a request-scoped database argument to Tablet insertion without changing the session's current database.
Changes
ITableSession.insert(Tablet tablet, String database).TSInsertTabletReq, including leader-cache grouping and redirects.InsertTabletStatement; the session and connection database remain unchanged.ITableSessionimplementations.Example
Before, changing the write target also changes session state:
After, each insertion can select its database without changing the current database:
Compatibility
The database is carried by a new optional Thrift field, so existing one-argument insert requests remain unchanged. As agreed for this change, the new overload relies on a same-version or newer server; an older server can ignore the unknown optional field and fall back to its session database.
Validation
./mvnw spotless:applysucceeded for the affected modules.git diff --checkpassed.