feat(context): adding ctx as first argument

[glimchb]

To maintain backward compatibility
addint WithContext set of functions
instead of changing the signatures of existing ones.

The Ctx parameter is not used yet in many functions yet.
The usage will come in the following PR after interface is changed.

Signed-off-by: Boris Glimcher Boris.Glimcher@emc.com

mentioned in #100 (comment)

[glimchb]

@philippgille if you like the direction, I will continue for the rest of the packages...

[philippgille]

Thanks for the draft, and in general for all your recent contributions! 💪

So this PR is one valid approach, and also one that other popular libraries have used in the past, when they moved from functions without context, towards functions with it. For example the Redis library v6 only had client.Do (here) and v7 then gained a client.DoContext method (here), giving people time to adapt to passing a context around, and in v8 they then did the breaking change to remove client.DoContext and change client.Do to require a context as first arg (here).
Other libraries followed a similar path.

The approach I originally had in mind was the rough one: Just change the signatures and willingly break backward compatibility. To make it a bit smoother, I was thinking of releasing a version that has updates for all stores to use the latest minor and patch versions of their dependencies, and update to latest supported Go versions. #108 and #109 were meant for that, but I wanted to have a more solid testing first before a release. I managed to switch from Travis CI to GitHub Actions in #110 and use Mage as build tool in #111, but one TODO I didn't get to was testing on Windows, as well as some other minor tasks.
But we could still go this route:

1. Update all deps (once again)
2. Update Go mod version to oldest still supported one (1.20 currently)
3. Test on Windows (either manually or configure CI to include it)
4. Other required fixes/cleanups/improvements
5. Release a version without breaking changes
6. Right after, release a new version with breaking changes.

That way no one feels forced to migrate to the newest one, as they still get all the latest dependencies in the former, and they have time to migrate the context passing.

Benefit on our side is that the gokv.Store interface stays maximally simple with its 3 functions (plus Close), which is one of the main goals.

---

But maybe there's another option, something in between: While checking the Redis changelogs between v6 and v9 for your other PR, I noticed that Redis introduced a client.WithContext function, which clones the client and sets a context on the client, after which then the old Do method could be called without context, while still taking the context into accont: https://github.com/redis/go-redis/blob/v7.4.1/redis.go#L559 + https://github.com/redis/go-redis/blob/v7.4.1/redis.go#L574
This way the gokv interface could stay the same (at least for one "intermediate" version until fully migrating to an interface with contexts), while still allowing people to call client.WithContext(ctx).Get(...) if they want to make use of a context object with timeouts already.
Downside is that it's costly to clone the client for each method call, and also ugly for users who want to use their context already and have to replace all their Get/Set/Delete calls by WithContext(ctx).Get/WithContext(ctx).Set/... especially if we plan to do the breaking change in a later version anyway, where they have to change the calls again.

---

I think I'm currently still leaning towards the hard cut, after a fresh release without context. But totally open to hear more opinions around this.

[glimchb]

@philippgille I'm fine with both hard-cut and mine WithContext approach as well

I really dislike the client.WithContext approach

I suggested this PR as I'm not aware of your timeline and release schedule when you want to do the hard cut...
So I just wanted to make some progress first without breaking compatibility...

---

another option is to leave the WithContext functions in the implementations but not in the interface... wdyt ?

---

anyways, thanks for the comments, as long as we making progress, I'm happy )))

[codecov-commenter]

Codecov Report

Attention: 36 lines in your changes are missing coverage. Please review.

Comparison is base (0b9cc3b) 56.69% compared to head (924f235) 62.04%.

Files	Patch %	Lines
mongodb/mongodb.go	0.00%	9 Missing ⚠️
mysql/mysql.go	0.00%	9 Missing ⚠️
tablestorage/tablestorage.go	0.00%	9 Missing ⚠️
tablestore/tablestore.go	0.00%	9 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
+ Coverage   56.69%   62.04%   +5.35%     
==========================================
  Files          11       25      +14     
  Lines         822     2274    +1452     
==========================================
+ Hits          466     1411     +945     
- Misses        327      752     +425     
- Partials       29      111      +82     

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

[glimchb]

Test on Windows (either manually or configure CI to include it)

done, see #139

[philippgille]

another option is to leave the WithContext functions in the implementations but not in the interface... wdyt ?

Huh that's an interesting idea! I'll think about it!

[glimchb]

@philippgille what are the next steps ? should I re-base or you taking a different approach ?

[philippgille]

Sorry for the delay, replied in #107 (reply in thread)

[glimchb]

@philippgille congrats on the new release! what is the decision on Context ?

[philippgille]

@philippgille congrats on the new release! what is the decision on Context ?

Yes, let's move forward as outlined in the discussion.

Regarding the implementation itself, I like the approach of making the *WithContext methods the main one, and the old ones call them with a context they create (some with a timeout that's configured in the store options, some without if the store options don't have any). I think that's what you've done so far here, so that's perfect 👍
The unit tests would ideally call both - this might require some changes in the gokv/test package.
To make the PRs easier to review, WDYT about having one PR per store implementation?

Thanks for working on this! 💪

[glimchb]

Regarding the implementation itself, I like the approach of making the *WithContext methods the main one, and the old ones call them with a context they create (some with a timeout that's configured in the store options, some without if the store options don't have any). I think that's what you've done so far here, so that's perfect 👍

Great! @philippgille
So this PR already addresses all the implementations and looks mergeable.
Please, clarify - what is missing ? I can add whatever is needed to move this forward...

[philippgille]

So this PR already addresses all the implementations and looks mergeable.
Please, clarify - what is missing ? I can add whatever is needed to move this forward...

• Would you say no additional unit tests are required, as testing the old methods implies calling the new ones implicitly?
• For the stores that don't pass on the context, we should probably still check it, at least when network or disk I/O is involved. For example in the bbolt implementation, we could check if the context was already canceled before a Get/Set/Delete, and potentially again afterwards.
• Currently the PR includes changes to the Store interface. I liked your idea of only changing the implementations first, while keeping the interface as is. But OTOH everyone who uses the interface, like as a parameter so others can inject the implementation of their choice, will then not benefit from the new methods yet. But still, it could be a follow-up PR before a new release.

[glimchb]

Thank you for calrifications!

• Would you say no additional unit tests are required, as testing the old methods implies calling the new ones implicitly?

Correct. Both old and new methods are covered by same tests.

• For the stores that don't pass on the context, we should probably still check it, at least when network or disk I/O is involved. For example in the bbolt implementation, we could check if the context was already canceled before a Get/Set/Delete, and potentially again afterwards.

Agree completely. Once this is merged, I will add context checking everywhere.

• Currently the PR includes changes to the Store interface. I liked your idea of only changing the implementations first, while keeping the interface as is. But OTOH everyone who uses the interface, like as a parameter so others can inject the implementation of their choice, will then not benefit from the new methods yet. But still, it could be a follow-up PR before a new release.

Yep, I can do that after this is merged.

[philippgille]

Something else came to mind: With some store configs having a timeout parameter, we should either document that they are completely ignored, or in the WithContext methods we could check for an existing ctx timeout, and when one is set we leave it as is, but when none is set we derive a new ctx with the timeout from the store config.

[glimchb]

Something else came to mind: With some store configs having a timeout parameter, we should either document that they are completely ignored, or in the WithContext methods we could check for an existing ctx timeout, and when one is set we leave it as is, but when none is set we derive a new ctx with the timeout from the store config.

I can add timeout to all store configs if you want, I was doing this for some already, and can expand to the rest...

[glimchb]

@philippgille let’s continue on this one ?

[philippgille]

Sorry for the long absence, I was focused a bit on a different project. 🙇‍♂️

Something else came to mind: With some store configs having a timeout parameter, we should either document that they are completely ignored, or in the WithContext methods we could check for an existing ctx timeout, and when one is set we leave it as is, but when none is set we derive a new ctx with the timeout from the store config.

I can add timeout to all store configs if you want, I was doing this for some already, and can expand to the rest...

No that's not necessary. With the ctx version of the interface being the future, users should configure the timeout on the ctx. That's also how most other libraries do it. And for the non-ctx methods you already correctly add the timeout to the ctx before calling the ctx method (in the store implementations whose configs have a timeout field).

I was more thinking of users who for example use the etcd store, and so far they had a timeout configured via store config, and called for example Set(k, v) with the correct expectation that the config's timeout is taken into account. Now they migrate to SetWithContext(ctx, k, v), and don't set a timeout on the ctx, but still have a timeout set in the store config. I assume their expectation would be for the timeout to have an effect. So I think in the ctx methods we need to check if the ctx has a timeout set, and if not, then set it based on the store config. Otherwise it's a silent behavior change that the users won't notice early, and potentially only run into issues in production environments.

Later, in a future version, when we do the breaking change of removing the non-ctx methods (to keep the interface small and simple), we could remove the timeout fields from the config, and users would then get compile errors and then set the timeout on the ctx, instead of accidentally forgetting it.

WDYT?