[BugFix]support table function on compute nodes

[abc982627271]

Fixes #issue

What type of PR is this:

• BugFix
• Feature
• Enhancement
• Refactor
• UT
• Doc
• Tool

Does this PR entail a change in behavior?

• Yes, this PR will result in a change in behavior.
• No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

• Interface/UI changes: syntax, type conversion, expression evaluation, display information
• Parameter changes: default values, similar parameters but with different default values
• Policy changes: use new policy to replace old one, functionality automatically enabled
• Feature removed
• Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

• I have added test cases for my bug fix or my new feature
• This pr needs user documentation (for new or modified features or behaviors)
  • I have added documentation for my new feature or new function

Bugfix cherry-pick branch check:

• I have checked the version labels which the pr will be auto-backported to the target branch
  • 3.2
  • 3.1
  • 3.0
  • 2.5

[starrocks-cr]

Code reviews can vary in depth and focus depending on the context of the code change and what the reviewers are looking for. Here’s a summary of potential suggestions for this particular diff snippet review:

1. Consistent Naming:

   • The renaming from Backend to ComputeNode might be implying that Backend has been either replaced or extended by ComputeNode. It's crucial to verify everywhere in the codebase that this change is consistently applied and documented if it is part of a larger refactoring.

2. Exception Message Clarity:

   • The new exception message "Failed to send proxy request. No alive backends" could potentially be made more clear, as you're not just looking for "backends" anymore due to the shared-nothing addition but also "compute nodes." Adjusting the messages to reflect the current state of the system could reduce debugging time in the future.

3. Feature Toggles or Runtime Configuration:

   • Adding conditional logic based on RunMode.getCurrentRunMode() seems like it might be introducing runtime configuration control flow, which can be both powerful and risky. Ensure there is enough test coverage for both branches (shared-nothing and otherwise) and that these runtime changes are well-documented.

4. Error Handling and Retry Logic:

   • The adjustment adds new exception paths which could benefit from more sophisticated error handling. Depending on how critical this getFileSchema function is within the system, adding retry logic or better fallbacks might be worthwhile.

5. Thread Safety and Concurrency:

   • Collections.shuffle(nodeIds); implies a mutation of the nodeIds list. If nodeIds is shared across threads, this operation could introduce race conditions. Ensure that either nodeIds is confined to a single thread or is otherwise safely published between threads.

6. Code Duplication:

   • There appears to be some code duplication between the cases for handling when nodeIds is empty and when looking for backends or compute nodes. If similar logic is used in multiple places, consider abstracting it into a method to promote reuse and simplify future updates.

7. Unit Testing:

   • With logic being changed/added (especially around conditions like RunMode.getCurrentRunMode()), unit tests need to be updated or added to cover the new scenarios to ensure that the behavior is as expected.

8. Performance Implications:

   • By adding additional backend and compute node checks (as well as potentially making requests to each), there can be performance implications. Verify that this does not significantly impact the performance, especially in large deployments.

9. Documentation and Comments:

   • There aren't any comments in the provided diff. Contextual comments explaining why decisions were made (such as the introduction of different node types and run modes) can be incredibly valuable, particularly when unusual or non-obvious logic is introduced.

10. Dependency on Global State:

    • The overall logic relies heavily on the global state (GlobalStateMgr). Be aware that such reliance makes unit testing more difficult and may increase coupling, which can lead to issues with maintainability.

Without the broader context or understanding of prior and future pieces of work, these are top-level suggestions. Changes connected with databases, distributed systems, or resilient service design typically involve deeper considerations, which would be reviewed as part of a more thorough code base analysis.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

The version of Java (11.0.21) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

[github-actions]

[BE Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

[github-actions]

[FE Incremental Coverage Report]

✅ pass : 10 / 10 (100.00%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	com/starrocks/catalog/TableFunctionTable.java	10	10	100.00%	[]

[github-actions]

@Mergifyio backport branch-3.2

[github-actions]

@Mergifyio backport branch-3.1

[mergify]

backport branch-3.2

✅ Backports have been created

• #34931 [BugFix]support table function on compute nodes (backport #34577) has been created for branch branch-3.2

[mergify]

backport branch-3.1

✅ Backports have been created

• #34932 [BugFix]support table function on compute nodes (backport #34577) has been created for branch branch-3.1 but encountered conflicts