matrixone icon indicating copy to clipboard operation
matrixone copied to clipboard
verified publisher icon matrixorigin

chore: migrate query-service from cn to tn

Open xzxiong opened this issue 7 months ago • 2 comments

User description

What type of PR is this?

  • [ ] API-change
  • [ ] BUG
  • [ ] Improvement
  • [ ] Documentation
  • [x] Feature
  • [ ] Test and CI
  • [ ] Code Refactoring

Which issue(s) this PR fixes:

issue # https://github.com/matrixorigin/MO-Cloud/issues/5780

What this PR does / why we need it:

extend tn query-service with common setting.

PR Type

Enhancement

Description

  • Add query service handlers for system configuration

  • Implement Go runtime parameter controls (GOMAXPROCS, memory limit, GC)

  • Add file service cache management capabilities

  • Include metadata cache handling functionality

Changes walkthrough 📝

Relevant files
Enhancement
store.go
Add system configuration and cache management handlers     

pkg/tnservice/store.go

  • Add imports for runtime/debug, system, and objectio packages
  • Register 6 new query service handlers in initQueryCommandHandler
  • Implement handlers for Go runtime controls (GOMAXPROCS, memory limit,
    GC percent)
  • Add file service cache management handlers (resize and evict
    operations)
  • Implement metadata cache handler with capacity control and eviction
    		• +105/-0 
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.

    • xzxiong avatar Jun 26 '25 01:06 xzxiong

    You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    5780 - Not compliant

    Non-compliant requirements:

    • Fix error "strconv.ParseFloat: parsing "": invalid syntax" when using IS UNKNOWN with empty strings • Make empty string IS UNKNOWN return 0 (false) like MySQL • Make empty string IS NOT UNKNOWN return 1 (true) like MySQL • Handle 'NULL', ' ', 'TRUE', 'FALSE' strings with IS UNKNOWN/IS NOT UNKNOWN operators correctly

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 Security concerns

    Runtime control exposure:
    The PR exposes Go runtime controls (GOMAXPROCS, memory limit, GC percent) through query service handlers without apparent authentication or authorization checks. This could allow unauthorized users to modify critical runtime parameters and potentially cause denial of service or performance degradation.

    ⚡ Recommended focus areas for review

    Wrong PR

    This PR implements query service handlers for system configuration and runtime controls, but the linked ticket is about fixing IS UNKNOWN operator with empty strings. The code changes are completely unrelated to the reported bug.

     
    s.queryService.AddHandleFunc(query.CmdMethod_GOMAXPROCS, s.handleGoMaxProcs, false)
s.queryService.AddHandleFunc(query.CmdMethod_GOMEMLIMIT, s.handleGoMemLimit, false)
s.queryService.AddHandleFunc(query.CmdMethod_GOGCPercent, s.handleGoGCPercent, false)
s.queryService.AddHandleFunc(query.CmdMethod_FileServiceCache, s.handleFileServiceCacheRequest, false)
s.queryService.AddHandleFunc(query.CmdMethod_FileServiceCacheEvict, s.handleFileServiceCacheEvictRequest, false)
s.queryService.AddHandleFunc(query.CmdMethod_MetadataCache, s.handleMetadataCacheRequest, false)

    qodo-code-review[bot] avatar Jun 26 '25 01:06 qodo-code-review[bot]

    You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Validate non-negative memory limit

    Add validation to prevent setting negative memory limits which could cause
    runtime panics. The debug.SetMemoryLimit function expects non-negative values.

    pkg/tnservice/store.go [540-550]

     func (s *store) handleGoMemLimit(
 	ctx context.Context, req *query.Request, resp *query.Response, _ *morpc.Buffer,
 ) error {
+	if req.GoMemLimitRequest.MemLimitBytes < 0 {
+		return errors.New("MemLimitBytes cannot be negative")
+	}
 	resp.GoMemLimitResponse.MemLimitBytes = int64(debug.SetMemoryLimit(req.GoMemLimitRequest.MemLimitBytes))
 	s.rt.Logger().Info("QueryService::GoMemLimit",
 		zap.String("op", "set"),
 		zap.Int64("in", req.GoMemLimitRequest.MemLimitBytes),
 		zap.Int64("out", resp.GoMemLimitResponse.MemLimitBytes),
 	)
 	return nil
 }
    • [ ] Apply / Chat <!-- /improve --apply_suggestion=0 -->
    Suggestion importance[1-10]: 8

    __

    Why: This is a valuable suggestion as the Go documentation for debug.SetMemoryLimit states that a negative limit is illegal. Passing a negative value could lead to a runtime panic. Adding this validation prevents a potential crash and improves the service's stability.

    		Medium
    Validate positive MaxProcs value

    Add validation to ensure MaxProcs is positive before calling SetGoMaxProcs.
    Setting GOMAXPROCS to zero or negative values can cause runtime issues or
    unexpected behavior.

    pkg/tnservice/store.go [528-538]

     func (s *store) handleGoMaxProcs(
 	ctx context.Context, req *query.Request, resp *query.Response, _ *morpc.Buffer,
 ) error {
+	if req.GoMaxProcsRequest.MaxProcs <= 0 {
+		return errors.New("MaxProcs must be positive")
+	}
 	resp.GoMaxProcsResponse.MaxProcs = int32(system.SetGoMaxProcs(int(req.GoMaxProcsRequest.MaxProcs)))
 	s.rt.Logger().Info("QueryService::GoMaxProcs",
 		zap.String("op", "set"),
 		zap.Int32("in", req.GoMaxProcsRequest.MaxProcs),
 		zap.Int32("out", resp.GoMaxProcsResponse.MaxProcs),
 	)
 	return nil
 }
    • [ ] Apply / Chat <!-- /improve --apply_suggestion=1 -->
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly points out that the MaxProcs value should be validated. While system.SetGoMaxProcs might handle non-positive values gracefully by not changing the setting, explicitly checking for positive values and returning an error provides clearer feedback to the caller and makes the handler more robust.

    		Medium
    General
    Handle unknown cache types

    Add a default case to handle unknown cache types and return an error. This
    prevents silent failures when unsupported cache types are provided.

    pkg/tnservice/store.go [563-580]

     func (s *store) handleFileServiceCacheRequest(
 	ctx context.Context, req *query.Request, resp *query.Response, _ *morpc.Buffer,
 ) error {
 
 	if n := req.FileServiceCacheRequest.CacheSize; n > 0 {
 		switch req.FileServiceCacheRequest.Type {
 		case query.FileServiceCacheType_Disk:
 			fileservice.GlobalDiskCacheSizeHint.Store(n)
 		case query.FileServiceCacheType_Memory:
 			fileservice.GlobalMemoryCacheSizeHint.Store(n)
+		default:
+			return errors.New("unsupported cache type")
 		}
 		s.rt.Logger().Info("cache size adjusted",
 			zap.Any("type", req.FileServiceCacheRequest.Type),
 			zap.Any("size", n),
 		)
 	}
 	return nil
 }
    • [ ] Apply / Chat <!-- /improve --apply_suggestion=2 -->
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies a potential issue where an unsupported cache type would be silently ignored. Adding a default case to the switch statement to handle unknown types and return an error makes the function more robust and prevents silent failures.

    		Medium
    • [ ] More <!-- /improve --more_suggestions=true -->

    qodo-code-review[bot] avatar Jun 26 '25 01:06 qodo-code-review[bot]