Re: [PR] Fix incorrect DataNode prefix for alphanumeric sharding suffixes [shardingsphere]

["lliyuu520 (via GitHub)" <gi...@apache.org>]

lliyuu520 commented on PR #39952:
URL: https://github.com/apache/shardingsphere/pull/39952#issuecomment-5936686440

   Thanks for the precise review — the P3 wrapping issue is fixed in 31d52bca.
   
   On the P2 contract question, may I share some context from production? The issue was never really about whether the suffix consists of digits or letters — it is that the current derivation cannot locate the tenant suffix within the actual table name: the pattern matches nothing, the whole table name ends up as the prefix, binding-table validation breaks, and joins fan out to every tenant's table. That is the failure reported in #39875, and #35293 shows the same configuration arises naturally in practice. Because of this, I believe restricting binding tables to numeric endings — however understandable for keeping the contract simple — sidesteps the real problem; with the derivation fixed, both suffix styles can be handled uniformly.
   
   Since this PR already makes prefix derivation deterministic for separator-led alphanumeric suffixes (byte-identical behavior for digit endings, including multi-segment ones), the binding check could require that all actual tables of each binding table derive the same `DataNodeInfo` prefix, instead of requiring a digit ending. Concretely:
   
   - Replace the last-char-is-digit rule in `ShardingRuleChecker#hasValidNumericSuffix` with a shared-derived-prefix check consistent with `DATA_NODE_SUFFIX_PATTERN`;
   - Flip `ShardingRuleTest#assertCreateBindingTablesWithAlphabeticalSuffixesFailure` into a success assertion (keeping a failure case for genuinely inconsistent prefixes);
   - Update the binding-table documentation ("Keep the digits at the end exclusively for sharding suffixes") accordingly.
   
   For safety, the zero-padding name generation in `ShardingAutoTableAlgorithmUtils#findMatchedTargetName` is only reachable from auto-table algorithms, whose generated names are always digit-suffixed, so classic binding tables with letter suffixes never reach it.
   
   Would you be open to extending the contract in this direction — either in this PR or a follow-up, whichever you prefer? If you'd rather keep the numeric-only binding contract for now, that's perfectly fine too: I can re-scope this PR to the derivation fix alone and open a separate issue to discuss the contract.
   


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org