metadata: move FromOutgoingContextRaw() to internal #6765

Aditya-Sood · 2023-11-04T04:37:56Z

RELEASE NOTES: none

Aditya-Sood · 2023-11-04T04:39:55Z

hi @dfawley
so I've moved the method to internal/metadata and exported the required types from the top-level metadata package
kindly let me know if any further changes are required
thanks

codecov · 2023-11-04T04:41:40Z

Codecov Report

Merging #6765 (26a8b06) into master (ddd377f) will increase coverage by 0.31%.
The diff coverage is 100.00%.

❗ Current head 26a8b06 differs from pull request most recent head 4494626. Consider uploading reports for the commit 4494626 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6765      +/-   ##
==========================================
+ Coverage   83.51%   83.83%   +0.31%     
==========================================
  Files         287      287              
  Lines       30920    30837      -83     
==========================================
+ Hits        25824    25852      +28     
+ Misses       4020     3939      -81     
+ Partials     1076     1046      -30

Files	Coverage Δ
internal/transport/http2_client.go	`90.92% <100.00%> (+0.30%)`	⬆️
metadata/metadata.go	`89.55% <100.00%> (+0.15%)`	⬆️
stream.go	`81.39% <100.00%> (ø)`

... and 29 files with indirect coverage changes

Aditya-Sood · 2023-11-04T06:51:16Z

I've added comments to the now exported types and an additional check on the iterations over RawMD.Added slice (on top of the "length is even" check)

the current changes effectively trade the exposition of FromOutgoingContextRaw() for two internal types becoming exported

I couldn't come up with a cleaner solution than this, since we can't export types/methods from internal/metadata; and trying to share anything from the top-level metadata package will automatically make it public

would be happy to implement a cleaner solution if you have any ideas for it

dfawley · 2023-11-14T17:51:52Z

metadata/metadata.go

+// MDOutgoingKey is used as the key for outgoing metadata in ctx if it exists.
+// For gRPC-internal use ONLY.
+type MDOutgoingKey struct{}


Please move this to internal/metadata and reference it from there. Ideally users should never see any of our internal symbols at all.

dfawley · 2023-11-14T17:52:14Z

metadata/metadata.go

+// RawMD stores the original outgoing metadata (MD),
+// and the lists of kv Pairs appended to it (Added).
+// For gRPC-internal use ONLY.
+type RawMD struct {


Same with this please.

hi @dfawley
moving it to internal/metadata will create a cyclic-dependency, since RawMD.MD comes from metadata

shall I move it to the top-level internal directory instead? this will also require defining an internal.MD type which will become the type definition of metadata.MD

kindly let me know

MD is just a map [string][]string. You can redefine it in the internal/metadata package and everything should just work.

I don't think redefining metadata.MD will work because there are functions in internal/metadata that reference metadata.MD in their signatures - like func Validate(md metadata.MD) error and func Get(addr resolver.Address) metadata.MD

modifying these to use a newly defined internal.MD would require changes from all callers of these functions - is that feasible?

Oh, I see. metadata should ideally import internal/metadata and not the other way around. I didn't realize the existing internal/metadata already imports metadata.

How about this, instead, to keep things simple:

package internal var ( // we already have a ton of these..... FromOutgoingContextRaw any // func(ctx context.Context) (metadata.MD, [][]string, bool) ) --- package metadata import "internal" func init() { internal.FromOutgoingContextRaw = fromOutgoingContextRaw } func fromOutgoingContextRaw(ctx) (...) { /* impl */ }

github-actions · 2023-11-22T04:45:47Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

Aditya-Sood · 2023-11-27T02:45:49Z

hi @dfawley, could you please clarify on #6765 (comment)?

easwars · 2023-12-06T18:10:24Z

hi @dfawley, could you please clarify on #6765 (comment)?

Looks like @dfawley has provided clarification. Could you please take a look @Aditya-Sood. Thanks.

github-actions · 2023-12-12T18:45:47Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

…l method

dfawley

sorry I was caught up at work past few days

No worries! Thank you for your effort.

dfawley · 2023-12-14T17:06:50Z

internal/internal.go

@@ -188,6 +188,9 @@ var (
 	ExitIdleModeForTesting any // func(*grpc.ClientConn) error

 	ChannelzTurnOffForTesting func()
+
+  // Gets the function definition from metadata package


Please fix the formatting.

dfawley · 2023-12-14T17:08:41Z

internal/transport/http2_client.go

@@ -568,7 +568,8 @@ func (t *http2Client) createHeaderFields(ctx context.Context, callHdr *CallHdr)
 		headerFields = append(headerFields, hpack.HeaderField{Name: "grpc-trace-bin", Value: encodeBinHeader(b)})
 	}

-	if md, added, ok := metadata.FromOutgoingContextRaw(ctx); ok {
+	fromOutgoingCtxRaw := internal.FromOutgoingContextRaw.(func(context.Context) (metadata.MD, [][]string, bool))


Please instead do:

package transport var metadataFromOutgoingContextRaw = internal.FromOutgoingContextRaw.(func(context.Context) (metadata.MD, [][]string, bool))

and then use metadataFromOutgoingContextRaw directly. (This way we only do the type assertion once.)

dfawley · 2023-12-14T17:09:19Z

metadata/metadata.go

 //
 // This is intended for gRPC-internal use ONLY. Users should use
 // FromOutgoingContext instead.


This can be removed now.

dfawley · 2023-12-14T17:09:31Z

stream.go

@@ -184,7 +184,8 @@ func newClientStream(ctx context.Context, desc *StreamDesc, cc *ClientConn, meth
 	// when the RPC completes.
 	opts = append([]CallOption{OnFinish(func(error) { cc.idlenessMgr.OnCallEnd() })}, opts...)

-	if md, added, ok := metadata.FromOutgoingContextRaw(ctx); ok {
+	fromOutgoingCtxRaw := internal.FromOutgoingContextRaw.(func(context.Context) (metadata.MD, [][]string, bool))


Same as above with the transport package.

github-actions · 2023-12-20T18:45:48Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

dfawley

Thanks!

arvindbr8 · 2024-01-09T22:19:22Z

internal/internal.go

@@ -188,6 +188,9 @@ var (
 	ExitIdleModeForTesting any // func(*grpc.ClientConn) error

 	ChannelzTurnOffForTesting func()
+
+	// Gets the function definition from metadata package


Please fix formatting. We want the comment to start with the variable name. Also I feel like the comment doesnt explain what the method does, but instead talks about why this internal variable is being used.

how about something like this?

Suggested change

// Gets the function definition from metadata package

// FromOutgoingContextRaw returns the un-merged, intermediary contents of metadata.rawMD.

thanks @arvindbr8, I've merged your commit suggestion

Co-authored-by: Arvind Bright <arvind.bright100@gmail.com>

dfawley · 2024-01-16T17:24:28Z

@Aditya-Sood Looks like gofmt needs to be run again.

arvindbr8 · 2024-01-16T19:18:34Z

@Aditya-Sood - changes looks good to me. I can approve once you push after running gofmt

Aditya-Sood · 2024-01-18T04:46:30Z

hi @dfawley @arvindbr8, I've resolved the merge conflict with master

also I tried running go fmt . and gofmt internal/internal.go -e, but neither had any effect on any of the files - is there any other gofmt command you would like me to run?

arvindbr8

@Aditya-Sood -- all tests are green now. Approved!

arvindbr8 · 2024-01-18T17:55:28Z

Thanks for your contribution @Aditya-Sood!

dfawley self-requested a review November 7, 2023 18:14

dfawley self-assigned this Nov 7, 2023

dfawley reviewed Nov 14, 2023

View reviewed changes

dfawley assigned Aditya-Sood and unassigned dfawley Nov 14, 2023

dfawley added the Status: Requires Reporter Clarification label Nov 14, 2023

dfawley added this to the 1.60 Release milestone Nov 14, 2023

arvindbr8 modified the milestones: 1.60 Release, 1.61 Release Nov 14, 2023

github-actions bot added the stale label Nov 22, 2023

github-actions bot removed the stale label Nov 27, 2023

dfawley assigned dfawley and unassigned Aditya-Sood Nov 29, 2023

dfawley removed the Status: Requires Reporter Clarification label Nov 29, 2023

dfawley assigned Aditya-Sood and unassigned dfawley Dec 4, 2023

easwars added Type: Internal Cleanup Refactors, etc Status: Requires Reporter Clarification labels Dec 6, 2023

github-actions bot added stale and removed stale labels Dec 12, 2023

Increment: Redefines metatdata.FromOutgoingContextRaw() as an interna…

c7d0145

…l method

Aditya-Sood force-pushed the 4487-metadata-internal-method branch from 6061ea3 to c7d0145 Compare December 14, 2023 12:08

Merge branch 'master' into 4487-metadata-internal-method

208baa9

dfawley requested changes Dec 14, 2023

View reviewed changes

github-actions bot added stale and removed stale labels Dec 20, 2023

Code Review Changes

2b34f91

dfawley removed the Status: Requires Reporter Clarification label Dec 22, 2023

dfawley assigned dfawley and unassigned Aditya-Sood Dec 22, 2023

dfawley approved these changes Jan 9, 2024

View reviewed changes

dfawley assigned arvindbr8 and unassigned dfawley Jan 9, 2024

dfawley changed the title ~~metadata: moves FromOutgoingContextRaw() to internal/metadata~~ metadata: move FromOutgoingContextRaw() to internal Jan 9, 2024

arvindbr8 requested changes Jan 9, 2024

View reviewed changes

arvindbr8 assigned Aditya-Sood and unassigned arvindbr8 Jan 9, 2024

Refactor: Fix comment for FromOutgoingContextRaw definition

26a8b06

Co-authored-by: Arvind Bright <arvind.bright100@gmail.com>

dfawley assigned arvindbr8 and unassigned Aditya-Sood Jan 16, 2024

arvindbr8 added the Status: Requires Reporter Clarification label Jan 16, 2024

arvindbr8 assigned Aditya-Sood and unassigned arvindbr8 Jan 16, 2024

Merge branch 'master' into 4487-metadata-internal-method

4494626

arvindbr8 approved these changes Jan 18, 2024

View reviewed changes

arvindbr8 merged commit 987df13 into grpc:master Jan 18, 2024
12 checks passed

arvindbr8 removed the Status: Requires Reporter Clarification label Jan 18, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

metadata: move FromOutgoingContextRaw() to internal #6765

metadata: move FromOutgoingContextRaw() to internal #6765

Aditya-Sood commented Nov 4, 2023

Aditya-Sood commented Nov 4, 2023

codecov bot commented Nov 4, 2023 •

edited

Loading

Aditya-Sood commented Nov 4, 2023

dfawley Nov 14, 2023

dfawley Nov 14, 2023

Aditya-Sood Nov 16, 2023

dfawley Dec 4, 2023

Aditya-Sood Dec 5, 2023

dfawley Dec 5, 2023

github-actions bot commented Nov 22, 2023

Aditya-Sood commented Nov 27, 2023

easwars commented Dec 6, 2023

github-actions bot commented Dec 12, 2023

dfawley left a comment

dfawley Dec 14, 2023

dfawley Dec 14, 2023

dfawley Dec 14, 2023

dfawley Dec 14, 2023

github-actions bot commented Dec 20, 2023

dfawley left a comment

arvindbr8 Jan 9, 2024

Aditya-Sood Jan 15, 2024

dfawley commented Jan 16, 2024

arvindbr8 commented Jan 16, 2024

Aditya-Sood commented Jan 18, 2024

arvindbr8 left a comment

arvindbr8 commented Jan 18, 2024

	// Gets the function definition from metadata package
	// FromOutgoingContextRaw returns the un-merged, intermediary contents of metadata.rawMD.

metadata: move FromOutgoingContextRaw() to internal #6765

metadata: move FromOutgoingContextRaw() to internal #6765

Conversation

Aditya-Sood commented Nov 4, 2023

Aditya-Sood commented Nov 4, 2023

codecov bot commented Nov 4, 2023 • edited Loading

Codecov Report

Aditya-Sood commented Nov 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 22, 2023

Aditya-Sood commented Nov 27, 2023

easwars commented Dec 6, 2023

github-actions bot commented Dec 12, 2023

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 20, 2023

dfawley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley commented Jan 16, 2024

arvindbr8 commented Jan 16, 2024

Aditya-Sood commented Jan 18, 2024

arvindbr8 left a comment

Choose a reason for hiding this comment

arvindbr8 commented Jan 18, 2024

codecov bot commented Nov 4, 2023 •

edited

Loading