zhangshenghang commented on PR #10351:
URL: https://github.com/apache/seatunnel/pull/10351#issuecomment-3757777584
<!-- code-pr-reviewer -->
This PR mixes two independent changes: documentation restructuring and an
HBase bug fix. Please split them into separate PRs.
**[MAJOR] Documentation URL breaking change without redirects**
The PR renames `connector-v2` to `connectors` without configuring redirects.
All external references (GitHub issues, blogs, tutorials) will return 404
errors, impacting SEO and user bookmarks.
**Evidence**: `git diff --name-status dev` shows `R100 docs/en/connector-v2
→ docs/en/connectors`
**Impact**: SEO weight loss, broken external links, bookmark failures
**Suggestion**: Add 301 redirects in `docusaurus.config.js`:
```javascript
redirects: [
{
from: '/docs/en/connector-v2/*',
to: '/docs/en/connectors/:splat',
permanent: true,
},
{
from: '/docs/zh/connector-v2/*',
to: '/docs/zh/connectors/:splat',
permanent: true,
},
]
```
**[MAJOR] PR combines two unrelated changes**
The PR title indicates "Doc Structure Adjustment" but includes HBase
connector bug fixes (`HbaseClient.isExistsData` method changes and test
deletion).
**Evidence**:
`seatunnel-connectors-v2/connector-hbase/src/main/java/org/apache/seatunnel/connectors/seatunnel/hbase/client/HbaseClient.java:310-321`
**Impact**: Violates "one PR, one change" best practice; increases review
difficulty; if docs need rework, it delays the bug fix merge
**Suggestion**: Split into two PRs:
- PR #10351-A: Doc restructure (`connector-v2` → `connectors`)
- PR #10351-B: HBase NPE fix (empty table scenario)
**[MINOR] Unit test deleted instead of updated**
The PR modifies `HbaseClient.isExistsData` but deletes the unit test file
instead of updating it to cover the new logic.
**Evidence**: `D
seatunnel-connectors-v2/connector-hbase/src/test/java/org/apache/seatunnel/connectors/seatunnel/hbase/client/HbaseClientTest.java`
**Impact**: Reduced test coverage; modified method lacks unit test protection
**Suggestion**: Restore and update the unit test to cover:
- Empty table scenario (`scanner.next()` returns `null`)
- Data present scenario (`scanner.next()` returns non-null result)
--
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]
