Skip to content
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.

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

Add context in state component #1708

Closed
wants to merge 1 commit into from
Closed

Conversation

pigletfly
Copy link
Contributor

@pigletfly pigletfly commented May 6, 2022

Signed-off-by: pigletfly [email protected]

Description

add context in state interface:

type Store interface {
	BulkStore
	Init(metadata Metadata) error
	Features() []Feature
	Delete(ctx context.Context, req *DeleteRequest) error
	Get(ctx context.Context, req *GetRequest) (*GetResponse, error)
	Set(ctx context.Context, req *SetRequest) error
	Ping(ctx context.Context) error
}

Issue reference

dapr/dapr#2716
#1601

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #[issue number]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#[issue number]

@pigletfly pigletfly requested review from a team as code owners May 6, 2022 01:30
@pigletfly pigletfly force-pushed the add-ctx-state branch 2 times, most recently from f7aaf9b to 37d4a32 Compare May 6, 2022 01:42
Copy link
Member

@berndverst berndverst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address linter failures. You may want to run the linter locally to speed up addressing linter issues.

@pigletfly
Copy link
Contributor Author

sure,can we merge #1685 first?

Copy link
Contributor

@ItalyPaleAle ItalyPaleAle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great contribution, using contexts greatly increases the reliability of code.

My concern is that I see that in many, if not most, components the context is passed but not actually used. If your method accepts a context, it's sort of expected that it will rely on it (ie. if the context is canceled, the method should return).

state/alicloud/tablestore/tablestore_test.go Outdated Show resolved Hide resolved
@pigletfly
Copy link
Contributor Author

I think this is a great contribution, using contexts greatly increases the reliability of code.

My concern is that I see that in many, if not most, components the context is passed but not actually used. If your method accepts a context, it's sort of expected that it will rely on it (ie. if the context is canceled, the method should return).

yes, it depends on third-party implements, if the SDK doesn't accept a context, then context is useless actually.

@ItalyPaleAle
Copy link
Contributor

ItalyPaleAle commented May 7, 2022

Well, it just needs to be implemented in a SDK-by-SDK way.

For example, for aerospike (just because it's the first one in alphabetic order), you can see a way to halt the request on a context cancellation here: aerospike/aerospike-client-go#255 (comment) The same technique could be used in other SDKs that don't implement context support at all.

For AWS Dynamo DB, the methods need to be changed to add the WithContext variant. For example GetItem -> GetItemWIthContext; PutItem -> PutItemWithContext

@pigletfly pigletfly force-pushed the add-ctx-state branch 3 times, most recently from 1a3cc57 to c133955 Compare May 13, 2022 06:53
@pigletfly
Copy link
Contributor Author

Well, it just needs to be implemented in a SDK-by-SDK way.

For example, for aerospike (just because it's the first one in alphabetic order), you can see a way to halt the request on a context cancellation here: aerospike/aerospike-client-go#255 (comment) The same technique could be used in other SDKs that don't implement context support at all.

For AWS Dynamo DB, the methods need to be changed to add the WithContext variant. For example GetItem -> GetItemWIthContext; PutItem -> PutItemWithContext

now if the third-party SDK has used context for cancellation, I just added the context in the calling method, if not, we should wait for the SDK support, is that right ? @ItalyPaleAle

@ItalyPaleAle
Copy link
Contributor

@pigletfly

Note that some SDKs have context support but the method is named different. For example, for AWS Dynamo DB, you have to add WithContext... For example, GetItem becomes GetItemWithContext. This may be the case for other SDKs.

Also please see my comment in the code. In tests (files ending in _tests.go) I think we should NOT use context.TODO() but rather context.Background() because it's not a "TODO" to fix in the future, but an actual choice.

@yaron2
Copy link
Member

yaron2 commented May 18, 2022

@pigletfly please fix the failing tests.

@yaron2
Copy link
Member

yaron2 commented May 23, 2022

ping @pigletfly

@pigletfly pigletfly force-pushed the add-ctx-state branch 2 times, most recently from 04d1b98 to 3f9c726 Compare May 25, 2022 08:34
@pigletfly pigletfly force-pushed the add-ctx-state branch 3 times, most recently from b241596 to 73e3066 Compare May 27, 2022 03:11
@pigletfly
Copy link
Contributor Author

The left failing tests are caused by the intermediate state.After this PR is merged, I will create a separate PR in dapr/dapr as well. @yaron2 @berndverst

@ItalyPaleAle
Copy link
Contributor

@pigletfly can you please address my review's comments too?

@ItalyPaleAle
Copy link
Contributor

@pigletfly with "address" I am asking that you please fix the issues, not just "resolve" the comments :)

  1. Please make sure that the context is actually used in all components. I have pointed out various examples of SDKs where method names need to be changed so they use a context. Without that work, this PR is incomplete and is actually giving maintainers more work in the future
  2. Please do not use context.TODO() in tests but use context.Background() instead. "TODO" is used when you are saying that in the future that needs to be changed

@yaron2
Copy link
Member

yaron2 commented Jun 7, 2022

ping @pigletfly

@pigletfly
Copy link
Contributor Author

@pigletfly with "address" I am asking that you please fix the issues, not just "resolve" the comments :)

  1. Please make sure that the context is actually used in all components. I have pointed out various examples of SDKs where method names need to be changed so they use a context. Without that work, this PR is incomplete and is actually giving maintainers more work in the future
  2. Please do not use context.TODO() in tests but use context.Background() instead. "TODO" is used when you are saying that in the future that needs to be changed

should we send PR to corresponding sdk repo?Take aerospike for an example,

func (aspike *Aerospike) Get(req *state.GetRequest) (*state.GetResponse, error) {
asKey, err := as.NewKey(aspike.namespace, aspike.set, req.Key)
if err != nil {
return nil, err
}
policy := &as.BasePolicy{}
if req.Options.Consistency == state.Strong {
policy.ReadModeAP = as.ReadModeAPAll
policy.ReadModeSC = as.ReadModeSCLinearize
} else {
policy.ReadModeAP = as.ReadModeAPOne
policy.ReadModeSC = as.ReadModeSCSession
}
record, err := aspike.client.Get(policy, asKey)
if err != nil {
, we add ctx in Get method, but aspike.client.Get doesn't accept ctx. @ItalyPaleAle

Signed-off-by: pigletfly <[email protected]>
@berndverst
Copy link
Member

berndverst commented Jun 13, 2022

@pigletfly just try your best to use ctx where available, and maybe we can fix upstream components in subsequent changes.

To avoid breaking tests, can you please add temporary methods that keep the old signature?

You added:

func (s *AliCloudTableStore) BulkGet(ctx context.Context, reqs []state.GetRequest) (bool, []state.BulkGetResponse, error)

Maybe can name this BulkGetWithContext for now?

and then we also should add

func (s *AliCloudTableStore) BulkGet(reqs []state.GetRequest) (bool, []state.BulkGetResponse, error) {
  return s.BulkGetWithContext(ctx.Background(), reqs)

}

This way we can avoid breaking certification tests. Please do something like this for every state store component where you added context.

Eventually we can then remove the old method and make the new method the default.

The best recommendation will be that we rename all methods to *WithContext and keep the old methods to call *WithContext setting the ctx to Background. Then we can import the latest contrib into dapr/dapr and update the interface to use the *WithContext methods instead. Once that is merged we can update the certification tests in contrib and remove the old methods (the ones without *WithContext) at that time.

This is the only way to avoid introducing test failures - which we do not want to do anymore.

@pigletfly
Copy link
Contributor Author

@pigletfly just try your best to use ctx where available, and maybe we can fix upstream components in subsequent changes.

To avoid breaking tests, can you please add temporary methods that keep the old signature?

You added:

func (s *AliCloudTableStore) BulkGet(ctx context.Context, reqs []state.GetRequest) (bool, []state.BulkGetResponse, error)

Maybe can name this BulkGetWithContext for now?

and then we also should add

func (s *AliCloudTableStore) BulkGet(reqs []state.GetRequest) (bool, []state.BulkGetResponse, error) {
  return s.BulkGetWithContext(ctx.Background(), reqs)

}

This way we can avoid breaking certification tests. Please do something like this for every state store component where you added context.

Eventually we can then remove the old method and make the new method the default.

The best recommendation will be that we rename all methods to *WithContext and keep the old methods to call *WithContext setting the ctx to Background. Then we can import the latest contrib into dapr/dapr and update the interface to use the *WithBackground methods instead. Once that is merged we can update the certification tests in contrib and remove the old methods (the ones without *WithContext) at that time.

This is the only way to avoid introducing test failures - which we do not want to do anymore.

Good idea !!!

@berndverst
Copy link
Member

@pigletfly just try your best to use ctx where available, and maybe we can fix upstream components in subsequent changes.
To avoid breaking tests, can you please add temporary methods that keep the old signature?
You added:

func (s *AliCloudTableStore) BulkGet(ctx context.Context, reqs []state.GetRequest) (bool, []state.BulkGetResponse, error)

Maybe can name this BulkGetWithContext for now?
and then we also should add

func (s *AliCloudTableStore) BulkGet(reqs []state.GetRequest) (bool, []state.BulkGetResponse, error) {
  return s.BulkGetWithContext(ctx.Background(), reqs)

}

This way we can avoid breaking certification tests. Please do something like this for every state store component where you added context.
Eventually we can then remove the old method and make the new method the default.
The best recommendation will be that we rename all methods to *WithContext and keep the old methods to call *WithContext setting the ctx to Background. Then we can import the latest contrib into dapr/dapr and update the interface to use the *WithBackground methods instead. Once that is merged we can update the certification tests in contrib and remove the old methods (the ones without *WithContext) at that time.
This is the only way to avoid introducing test failures - which we do not want to do anymore.

Good idea !!!

@pigletfly could you give this a try after we complete the 1.8 release? At this point we won't include it in this release.

@berndverst berndverst added this to the v1.9 milestone Jun 20, 2022
@dapr-bot
Copy link
Collaborator

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@dapr-bot dapr-bot added the stale label Jul 20, 2022
@dapr-bot
Copy link
Collaborator

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@dapr-bot dapr-bot closed this Jul 27, 2022
@daixiang0 daixiang0 reopened this Jul 28, 2022
@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #1708 (21a0827) into master (4322a22) will decrease coverage by 0.00%.
The diff coverage is 41.89%.

❗ Current head 21a0827 differs from pull request most recent head 3914671. Consider uploading reports for the commit 3914671 to get more accurate results

@@            Coverage Diff             @@
##           master    #1708      +/-   ##
==========================================
- Coverage   36.59%   36.59%   -0.01%     
==========================================
  Files         177      177              
  Lines       16222    16217       -5     
==========================================
- Hits         5937     5934       -3     
+ Misses       9617     9615       -2     
  Partials      668      668              
Impacted Files Coverage Δ
state/aerospike/aerospike.go 18.48% <ø> (ø)
state/alicloud/tablestore/tablestore.go 78.72% <ø> (ø)
state/aws/dynamodb/dynamodb.go 85.71% <ø> (ø)
state/azure/blobstorage/blobstorage.go 29.87% <0.00%> (ø)
state/azure/cosmosdb/cosmosdb.go 10.91% <ø> (ø)
state/azure/tablestorage/tablestorage.go 11.86% <ø> (ø)
state/cassandra/cassandra.go 25.97% <ø> (ø)
state/cockroachdb/cockroachdb.go 25.00% <0.00%> (ø)
state/couchbase/couchbase.go 19.62% <ø> (ø)
state/gcp/firestore/firestore.go 24.65% <0.00%> (+0.65%) ⬆️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7eb3b7...3914671. Read the comment docs.

@berndverst
Copy link
Member

@pigletfly do you have time to work on this?

Please update the various function to accept a context parameter. Then also create another function with the old signature so you don't break certification tests.

Example:
Update func (aspike *Aerospike) Set(req *state.SetRequest) error to func (aspike *Aerospike) Set(ctx context.Context, req *state.SetRequest) error

Then also add a new function with the old signature which calls the new function with context but passes in context.Todo() -- in this case Todo is fine because we will be deleting that function eventually.

func (aspike *Aerospike) Set(req *state.SetRequest) error {
    return aspike.Set(context.Todo(), req)
}

@dapr-bot dapr-bot removed the stale label Aug 2, 2022
@dapr-bot
Copy link
Collaborator

dapr-bot commented Sep 1, 2022

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@dapr-bot dapr-bot added the stale label Sep 1, 2022
@dapr-bot
Copy link
Collaborator

dapr-bot commented Sep 8, 2022

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@dapr-bot dapr-bot closed this Sep 8, 2022
@artursouza artursouza removed this from the v1.9 milestone Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants