Caideyipi commented on PR #18132:
URL: https://github.com/apache/iotdb/pull/18132#issuecomment-4899943418

   I found a few issues that should be addressed before merging:
   
   1. High: Relational insertTablets cannot auto-create a new table.
   
   In InsertTablets.validateTableSchema, the call to validateTableSchema passes 
allowCreateTable=false. This makes ITableSession.insertTablets(List<Tablet>) 
fail when inserting into a new table, while the existing single-tablet insert 
path can auto-create the table. The current integration tests all create the 
tables first, so this behavior difference is not covered. Please either align 
this with the single-tablet path or clearly document and test that batch tablet 
insertion requires the table to exist.
   
   2. Medium: The new insertRelationalTablets path bypasses table-model 
redirection.
   
   Session.insertRelationalTablets always sends the request through 
getDefaultSessionConnection().insertTablets(request) and ignores 
RedirectException. The existing insertRelationalTablet path uses the 
table-model leader cache and groups rows/tablets by endpoint when redirection 
is enabled. In a cluster, this new path can keep sending requests to the 
default node and will not refresh tableModelDeviceIdToEndpoint, which may 
offset the intended performance gain. Please consider reusing or extending the 
existing relational tablet redirection logic for the batch path.
   
   3. Low: Merge cost is recorded even when the insert fails.
   
   recordMergeTabletsCost is called from finally, so failed RPCs or execution 
failures still contribute to the auto-disable decision. This can permanently 
disable merge due to failure-path latency rather than actual merge overhead. It 
would be safer to record the cost only after a successful insert, or explicitly 
exclude failed attempts from the heuristic.
   
   Question: Is insertTablets intentionally not supposed to support auto table 
creation? If yes, the API documentation and tests should make that clear.


-- 
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]

Reply via email to