Add context in state component

[pigletfly]

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

https://github.com/dapr/dapr/issues/2716 https://github.com/dapr/components-contrib/issues/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:

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

[pigletfly]

sure,can we merge https://github.com/dapr/components-contrib/pull/1685 first?

[pigletfly]

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]

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: https://github.com/aerospike/aerospike-client-go/issues/255#issue-416695859 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]

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]

@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]

@pigletfly please fix the failing tests.

[yaron2]

ping @pigletfly

[pigletfly]

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]

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

[ItalyPaleAle]

@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]

ping @pigletfly

[pigletfly]

@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, https://github.com/dapr/components-contrib/blob/1af102f6f3dd3c96a0594e78e196e1917546904d/state/aerospike/aerospike.go#L165-L180 , we add ctx in Get method, but aspike.client.Get doesn't accept ctx. @ItalyPaleAle

[berndverst]

@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]

@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]

@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.

[dapr-bot]

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]

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!

[codecov[bot]]

Codecov Report

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

:exclamation: 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%)	:arrow_up:
... 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]

@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]

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]

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!