fix: Make datacatalog gRPC server MaxRecvMsgSize configurable #6313

honnix · 2025-03-06T16:09:51Z

Tracking issue

Why are the changes needed?

It is possible that a task might upload an artifact larger than 4MB that is default value of MaxRecvMsgSize of gRPC server, and when that happens propeller would get a failed call. It feels reasonable to leave the choice to users.

What changes were proposed in this pull request?

Make datacatalog gRPC server MaxRecvMsgSize configurable

How was this patch tested?

Partially unit tested

Labels

Please add one or more of the following labels to categorize your PR:

added: For new features.
changed: For changes in existing functionality.
deprecated: For soon-to-be-removed features.
removed: For features being removed.
fixed: For any bug fixed.
security: In case of vulnerabilities

This is important to improve the readability of release notes.

Setup process

Screenshots

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Related PRs

Docs link

Summary by Bito

This PR adds a configurable parameter (grpcMaxRecvMsgSizeMBs: 6) for gRPC server's maximum receive message size to address large artifact upload issues. It updates YAML and Go configurations, revises gRPC server initialization, and adds command flag support. The change enables larger artifact uploads without failed gRPC calls, improving system reliability.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 1

flyte-bot · 2025-03-06T16:10:16Z

Code Review Agent Run Status

Limitations and other issues: ❌ Failure - The AI Code Review Agent skipped reviewing this change because it is configured to exclude certain pull requests based on the source/target branch or the pull request status. You can change the settings here, or contact the agent instance creator at [email protected].

codecov · 2025-03-06T16:13:20Z

Codecov Report

Attention: Patch coverage is 10.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 58.50%. Comparing base (4f0a73b) to head (a58c9f9).
Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
datacatalog/pkg/rpc/datacatalogservice/service.go	0.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6313      +/-   ##
==========================================
+ Coverage   58.48%   58.50%   +0.01%     
==========================================
  Files         937      937              
  Lines       71088    71094       +6     
==========================================
+ Hits        41576    41590      +14     
+ Misses      26360    26352       -8     
  Partials     3152     3152

Flag	Coverage Δ
unittests-datacatalog	`59.03% <10.00%> (-0.04%)`	⬇️
unittests-flyteadmin	`56.30% <ø> (+0.02%)`	⬆️
unittests-flytecopilot	`30.99% <ø> (ø)`
unittests-flytectl	`64.76% <ø> (+0.05%)`	⬆️
unittests-flyteidl	`76.12% <ø> (ø)`
unittests-flyteplugins	`61.00% <ø> (ø)`
unittests-flytepropeller	`54.80% <ø> (+<0.01%)`	⬆️
unittests-flytestdlib	`64.02% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

honnix · 2025-03-06T16:16:49Z

datacatalog/pkg/config/config_flags.go

@@ -52,6 +52,7 @@ func (cfg Config) GetPFlagSet(prefix string) *pflag.FlagSet {
 	cmdFlags := pflag.NewFlagSet("Config", pflag.ExitOnError)
 	cmdFlags.Int(fmt.Sprintf("%v%v", prefix, "grpcPort"), defaultConfig.GrpcPort, "On which grpc port to serve Catalog")
 	cmdFlags.Bool(fmt.Sprintf("%v%v", prefix, "grpcServerReflection"), defaultConfig.GrpcServerReflection, "Enable GRPC Server Reflection")
+	cmdFlags.Int(fmt.Sprintf("%v%v", prefix, "grpcMaxRecvMsgSizeMBs"), defaultConfig.GrpcMaxRecvMsgSizeMBs, "The max receive message size; if unset defaults to gRPC server default value")


I actually modified this manually because make generate didn't work for me.

Installing github.com/vektra/mockery/[email protected] go: github.com/vektra/mockery/[email protected] (in github.com/vektra/mockery/[email protected]): go.mod:5: unknown directive: toolchain

I think something is not working well with goenv.

I had to use goenv shell.

Proposed a minor fix in #6314

flyte-bot · 2025-03-06T17:19:07Z

Code Review Agent Run #8929e2

Actionable Suggestions - 1

datacatalog/pkg/rpc/datacatalogservice/service.go - 1
- Consider checking for positive value instead · Line 142-142

Review Details

Files reviewed - 5 · Commit Range: d3e776d..d3e776d
- datacatalog/datacatalog_config.yaml
- datacatalog/pkg/config/config.go
- datacatalog/pkg/config/config_flags.go
- datacatalog/pkg/config/config_flags_test.go
- datacatalog/pkg/rpc/datacatalogservice/service.go
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by

flyte-bot · 2025-03-06T17:23:07Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Enhance YAML and Configurations	- `values-keycloak-idp-flyteclients-without-browser.yaml` - Added 'grpcMaxRecvMsgSizeMBs' set to 6 for client configuration. - `values.yaml` - Inserted 'grpcMaxRecvMsgSizeMBs' set to 6 to configure gRPC limits. - `values.yaml` - Configured 'grpcMaxRecvMsgSizeMBs' to 6 to update gRPC message size settings. - `datacatalog_config.yaml` - Introduced 'grpcMaxRecvMsgSizeMBs' set to 10 in the datacatalog config.
Bug Fix - Fix gRPC Server Config and Behavior	- `config.go` - Added new field 'GrpcMaxRecvMsgSizeMBs' and updated default configuration. - `config_flags.go` - Introduced a command flag for setting 'grpcMaxRecvMsgSizeMBs' on startup. - `service.go` - Modified gRPC server initialization to use the configurable max receive message size.
Testing - Add Tests for gRPC Flag Configuration	- `config_flags_test.go` - Appended unit tests to verify correct parsing and application of the 'grpcMaxRecvMsgSizeMBs' flag.

flyte-bot · 2025-03-06T17:23:08Z

datacatalog/pkg/rpc/datacatalogservice/service.go

 		),
-	)
+	)}
+	if cfg.GrpcMaxRecvMsgSizeMBs != -1 {


Consider checking for positive value instead

The condition cfg.GrpcMaxRecvMsgSizeMBs != -1 might not handle the case where GrpcMaxRecvMsgSizeMBs is 0, which could be interpreted as 'use default'. Consider checking if the value is greater than 0 instead.

Code suggestion

Check the AI-generated fix before applying

Suggested change

if cfg.GrpcMaxRecvMsgSizeMBs != -1 {

if cfg.GrpcMaxRecvMsgSizeMBs > 0 {

Code Review Run #8929e2

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

Yes, avoid them

Signed-off-by: Hongxin Liang <[email protected]>

flyte-bot · 2025-03-06T21:19:08Z

Code Review Agent Run #c55beb

Actionable Suggestions - 0

Review Details

Files reviewed - 5 · Commit Range: fd7b760..fd7b760
- datacatalog/datacatalog_config.yaml
- datacatalog/pkg/config/config.go
- datacatalog/pkg/config/config_flags.go
- datacatalog/pkg/config/config_flags_test.go
- datacatalog/pkg/rpc/datacatalogservice/service.go
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by

eapolinario

Can you also expose this in the flyte-core and flyte-binary charts?

Signed-off-by: Hongxin Liang <[email protected]>

honnix · 2025-03-11T22:06:55Z

Can you also expose this in the flyte-core and flyte-binary charts?

I did some changes in the last commit. Please let me know if I did something wrong or missed some. I'm not familiar with these things. Thank you.

flyte-bot · 2025-03-11T23:12:58Z

Code Review Agent Run #752826

Actionable Suggestions - 0

Review Details

Files reviewed - 3 · Commit Range: fd7b760..a58c9f9
- charts/flyte-core/values-keycloak-idp-flyteclients-without-browser.yaml
- charts/flyte-core/values.yaml
- charts/flyte/values.yaml
Files skipped - 2
- charts/flyte-core/README.md - Reason: Filter setting
- charts/flyte/README.md - Reason: Filter setting
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers a full AI review.

Refer to the documentation for additional commands.

Configuration

This repository uses code_review_bito You can customize the agent settings here or contact your Bito workspace admin at [email protected].

Documentation & Help

AI Code Review powered by

honnix added the fixed For any bug fixes label Mar 6, 2025

honnix force-pushed the honnix/datacatalog-max-rcv branch from 4ec063f to d3e776d Compare March 6, 2025 16:16

honnix commented Mar 6, 2025

View reviewed changes

honnix marked this pull request as ready for review March 6, 2025 16:28

honnix requested a review from hamersaw March 6, 2025 16:28

flyte-bot reviewed Mar 6, 2025

View reviewed changes

fix: Make datacatalog gRPC server MaxRecvMsgSize configurable

fd7b760

Signed-off-by: Hongxin Liang <[email protected]>

honnix force-pushed the honnix/datacatalog-max-rcv branch from d3e776d to fd7b760 Compare March 6, 2025 20:17

honnix requested a review from eapolinario March 7, 2025 09:59

eapolinario reviewed Mar 11, 2025

View reviewed changes

Chart values

a58c9f9

Signed-off-by: Hongxin Liang <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Make datacatalog gRPC server MaxRecvMsgSize configurable #6313

fix: Make datacatalog gRPC server MaxRecvMsgSize configurable #6313

honnix commented Mar 6, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Mar 6, 2025

codecov bot commented Mar 6, 2025 •

edited

Loading

honnix Mar 6, 2025

honnix Mar 6, 2025

honnix Mar 6, 2025

honnix Mar 6, 2025

flyte-bot commented Mar 6, 2025 •

edited

Loading

Code Review Agent Run #8929e2

flyte-bot commented Mar 6, 2025 •

edited

Loading

Changelist by Bito

flyte-bot Mar 6, 2025

flyte-bot commented Mar 6, 2025 •

edited

Loading

Code Review Agent Run #c55beb

eapolinario left a comment

honnix commented Mar 11, 2025

flyte-bot commented Mar 11, 2025 •

edited

Loading

Code Review Agent Run #752826

Bito Usage Guide

	if cfg.GrpcMaxRecvMsgSizeMBs != -1 {
	if cfg.GrpcMaxRecvMsgSizeMBs > 0 {

fix: Make datacatalog gRPC server MaxRecvMsgSize configurable #6313

Are you sure you want to change the base?

fix: Make datacatalog gRPC server MaxRecvMsgSize configurable #6313

Conversation

honnix commented Mar 6, 2025 • edited by flyte-bot Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Labels

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

Summary by Bito

flyte-bot commented Mar 6, 2025

Code Review Agent Run Status

codecov bot commented Mar 6, 2025 • edited Loading

Codecov Report

honnix Mar 6, 2025

Choose a reason for hiding this comment

honnix Mar 6, 2025

Choose a reason for hiding this comment

honnix Mar 6, 2025

Choose a reason for hiding this comment

honnix Mar 6, 2025

Choose a reason for hiding this comment

flyte-bot commented Mar 6, 2025 • edited Loading

Code Review Agent Run #8929e2

flyte-bot commented Mar 6, 2025 • edited Loading

Changelist by Bito

flyte-bot Mar 6, 2025

Choose a reason for hiding this comment

flyte-bot commented Mar 6, 2025 • edited Loading

Code Review Agent Run #c55beb

eapolinario left a comment

Choose a reason for hiding this comment

honnix commented Mar 11, 2025

flyte-bot commented Mar 11, 2025 • edited Loading

Code Review Agent Run #752826

Bito Usage Guide

honnix commented Mar 6, 2025 •

edited by flyte-bot

Loading

codecov bot commented Mar 6, 2025 •

edited

Loading

flyte-bot commented Mar 6, 2025 •

edited

Loading

flyte-bot commented Mar 6, 2025 •

edited

Loading

flyte-bot commented Mar 6, 2025 •

edited

Loading

flyte-bot commented Mar 11, 2025 •

edited

Loading