New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Server return with Attachment #2648

Merged

chickenlj merged 11 commits into apache:main from YarBor:main

Apr 26, 2024

Contributor

YarBor commented Mar 31, 2024 •

edited

Loading

fix: Server return with Attachment (#2641)

in this pr, every call will return all attachment-data , whether built-in data or user-attached data. in triple unray call

but in this issue
i have no idea to fix it in an elegant way

I tried adding new fields like (userAttachmentKey or whatever) but it's not very nice to separate the built-in data from the user-attached data. I also thought about modifying the key of user-attached data before transfer and restoring it after transfer, but that doesn't seem feasible in this question (called in curl mode).

there may need some good idea？

but in this issue
i have no idea to fix it in an elegant way

I tried adding new fields like (userAttachmentKey or whatever) but it's not very nice to separate the built-in data from the user-attached data. I also thought about modifying the key of user-attached data before transfer and restoring it after transfer, but that doesn't seem feasible in this question (called in curl mode).

there may need some good idea？

add new api

One possible point to discuss is that in the additional data received by both parties, the first letter of the key will be changed to a capital letter.

client-side-call

func rpcCaller(){
        header := http.Header{"testKey1": []string{"testVal1"}, "testKey2": []string{"testVal2"}}
        // to store outgoing data ,and reserve the location for the receive field.
        // header will be copy , and header's key will change to be lowwer. 
	ctx, _ := triple_protocol.NewOutgoingContext(context.Background(), header )

        res, err := cli.Greet(ctx, &greet.GreetRequest{Name: "triple"})
	// stream, err := cli.GreetServerStream(ctx, &greet.GreetServerStreamRequest{Name: "triple"})
        // stream, err := cli.GreetStream(ctx)
        // stream, err := cli.GreetClientStream(ctx)

        /* 
             do something and close connection.
        */ 

        // to get return data , and key is lowwer , like grpc.
      	data, err := triple_protocol.FromIncomingContext(ctx)
}

server-side

func (impl *Impl) ****call(ctx context.Context, *** api.Req) (*** , errno){
	data, _ := triple.FromIncomingContext(ctx)

        // do something

        triple.SetOutgoingData(ctx , http.Header{"OutgoingDataKey1" , []string{"OutgoingDataVal1"}})
	/* OR */ 
        triple.AppendToOutgoingContext(ctx, "OutgoingContextKey2", "OutgoingDataVal2")
}

new api applies to all 4 calling methods.


          Add : add extraData api in rpc calling

8ce359f

Signed-off-by: YarBor <yarbor.ww@gmail.com>

YarBor force-pushed the main branch from 5ff9818 to 8ce359f Compare

April 10, 2024 02:54

YarBor added 4 commits

April 10, 2024 14:08


          Add :Client side unray extradata send/return logic

ee88596

Signed-off-by: YarBor <yarbor.ww@gmail.com>


          fic ci

f45ff57

Signed-off-by: YarBor <yarbor.ww@gmail.com>


          feat:Implemented data transfer and return

6d78967

rewrite logic,Put the api into protocol/triple/triple_protocol layer
used triple/triple_protocol/header.go's logic

Signed-off-by: YarBor <yarbor.ww@gmail.com>


          fix ci and format import

4a1a220

Signed-off-by: YarBor <yarbor.ww@gmail.com>

YarBor force-pushed the main branch from 4b276bd to 4a1a220 Compare

April 10, 2024 12:08


          Merge branch 'apache:main' into main

d74e072

YarBor changed the title ~~fix: Server return with Attachment (#2641)~~ fix: Server return with Attachment

DMwangnima reviewed

View reviewed changes

protocol/triple/triple_protocol/header.go Outdated

               }
               // NewOutgoingContext sets headers entirely. If there are existing headers, they would be replaced.
               // It is used for passing headers to server-side.
               // It is like grpc.NewOutgoingContext.
               // Please refer to https://github.com/grpc/grpc-go/blob/master/Documentation/grpc-metadata.md#sending-metadata.
-              func NewOutgoingContext(ctx context.Context, header http.Header) context.Context {
-              	return context.WithValue(ctx, headerOutgoingKey{}, header)
+              func NewOutgoingContext(ctx context.Context, data interface{}) (context.Context, error) {

Contributor

DMwangnima Apr 14, 2024

Why do we need to modify the args of this function?

I think http.Header is clear and efficient enough for users to use.

Contributor Author

YarBor Apr 14, 2024

I will change it back.

protocol/triple/triple_protocol/header.go Outdated

               }
               // AppendToOutgoingContext merges kv pairs from user and existing headers.
               // It is used for passing headers to server-side.
               // It is like grpc.AppendToOutgoingContext.
               // Please refer to https://github.com/grpc/grpc-go/blob/master/Documentation/grpc-metadata.md#sending-metadata.
-              func AppendToOutgoingContext(ctx context.Context, kv ...string) context.Context {
+              func AppendToOutgoingContext(ctx context.Context, kv ...string) (context.Context, error) {

Contributor

DMwangnima Apr 14, 2024

What is the reason of returning error?

Contributor Author

YarBor Apr 14, 2024

I think this needs to be discussed. Should there be constraints on the attachment key? When transmitting and returning within the framework, the first letter of the attachment key will be changed to uppercase. On the receiving side, we cannot restore the original key.

protocol/triple/triple_protocol/header.go Outdated

-              func ExtractFromOutgoingContext(ctx context.Context) http.Header {
-              	headerRaw := ctx.Value(headerOutgoingKey{})
-              	if headerRaw == nil {
+              func ExtractFromOutgoingContext(ctx context.Context) map[string][]string {

Contributor

DMwangnima Apr 14, 2024

ditto.

Contributor Author

YarBor Apr 14, 2024

i will change it back.

protocol/triple/triple_protocol/header.go Outdated

               // Please refer to https://github.com/grpc/grpc-go/blob/master/Documentation/grpc-metadata.md#receiving-metadata-1.
-              func FromIncomingContext(ctx context.Context) (http.Header, bool) {
-              	header, ok := ctx.Value(headerIncomingKey{}).(http.Header)
+              func FromIncomingContext(ctx context.Context) (map[string][]string, bool) {

Contributor

DMwangnima Apr 14, 2024

ditto.

Contributor Author

YarBor Apr 14, 2024

i will change it back.

protocol/triple/triple_protocol/header.go Outdated

               	mergeHeaders(conn.RequestHeader(), header)
               	return conn.Send(nil)
               }
+              func outGoingKeyCheck(key string) bool {

Contributor

DMwangnima Apr 14, 2024

Where is this function used?

Contributor Author

YarBor Apr 14, 2024

ditto.

Contributor Author

YarBor Apr 14, 2024

i think it should be check.

DMwangnima reviewed

View reviewed changes

client/client.go Outdated

               	if err != nil {
               		return nil, err
               	}
-              	return conn.refOpts.invoker.Invoke(ctx, inv), nil
+              	res := conn.refOpts.invoker.Invoke(ctx, inv)

Contributor

DMwangnima Apr 14, 2024

It seems there is no need to change statement style.

Contributor Author

YarBor Apr 14, 2024

Yes, the change here is a mistake. This was a change while testing, I will change it back to how it was.

protocol/triple/triple_invoker.go Outdated

               	return &result
               }
+              func mergeAttachmentToOutgoing(ctx context.Context, attachments map[string]interface{}) (context.Context, error) {

Contributor

DMwangnima Apr 14, 2024

Since you extract attachments merge logic from parseInvocation. Why do not extract entire logics about processing attachments?

Contributor Author

YarBor Apr 14, 2024

i think u are right.

YarBor added 2 commits

April 15, 2024 09:33


          change api back

0d1eb75

Signed-off-by: YarBor <yarbor.ww@gmail.com>


          Change the writing rules of incoming/outgoing key-value pairs

Signed-off-by: YarBor <yarbor.ww@gmail.com>

codecov-commenter commented Apr 16, 2024 •

edited

Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.31%. Comparing base (1bd6abc) to head (1bd6abc).

❗ Current head 1bd6abc differs from pull request most recent head 57227f3. Consider uploading reports for the commit 57227f3 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2648   +/-   ##
=======================================
  Coverage   47.31%   47.31%           
=======================================
  Files         341      341           
  Lines       25165    25165           
=======================================
  Hits        11908    11908           
  Misses      12106    12106           
  Partials     1151     1151

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

YarBor added 2 commits

April 17, 2024 19:22


          Rewrite the logic of returning ExtraData, now the new api applies to …

d09d80f

…all 4 calling methods.

Signed-off-by: YarBor <yarbor.ww@gmail.com>


          fix comment

44965e4

Signed-off-by: YarBor <yarbor.ww@gmail.com>

YarBor force-pushed the main branch from b409434 to 44965e4 Compare

April 21, 2024 07:09


          Merge branch 'main' into main

57227f3

sonarqubecloud bot commented Apr 23, 2024

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

YarBor requested a review from DMwangnima

April 24, 2024 00:57

chickenlj approved these changes

View reviewed changes

Contributor

chickenlj left a comment

LGTM.

chickenlj merged commit 72f80ae into apache:main

5 checks passed

EndlessSeeker mentioned this pull request

Community weekly meeting record (2024.04.26) #2667

Open

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet