Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

fix: fix several issues in eventually_consistent #26

Merged

Conversation

tmatsuo
Copy link
Contributor

@tmatsuo tmatsuo commented Apr 4, 2020

  • correctly handle argument provided to the decorators
  • provide the real callback for retry_on_exception instead of None

fixes #25

The helper function _retry_on_exception didn't return the inner
function properly so that, it is always None.

Luckily, if you pass None to retry_on_exception argument, the
retrying module always retries as shown bellow:

https://github.com/rholder/retrying/blob/1d5699348d707e377aad7488da6a8a1b48a65933/retrying.py#L139

Previously, you can not pass any arguments to the decorator mark and call.
This PR will allow both syntax with or without arguments.

I also added unit tests to this module. I'm not sure if we have CI
builds, but the test passes locally.

Takashi Matsuo added 2 commits April 4, 2020 14:48
* correctly handle argument provided to the decorators
* provide the real callback for retry_on_exception instead of None

fixes GoogleCloudPlatform#25

The helper function `_retry_on_exception` didn't return the inner
function properly so that, it is always None.

Luckily, if you pass `None` to `retry_on_exception` argument, the
retrying module always retries as shown bellow:

https://github.com/rholder/retrying/blob/1d5699348d707e377aad7488da6a8a1b48a65933/retrying.py#L139

Previously, you can not pass any arguments to the decorator `mark` and `call`.
This PR will allow both syntax with or without arguments.

I also added unit tests to this module. I'm not sure if we have CI
builds, but the test passes locally.
@tmatsuo
Copy link
Contributor Author

tmatsuo commented Apr 4, 2020

@busunkim96 @leahecole

PTAL

Copy link
Contributor

@leahecole leahecole left a comment

Choose a reason for hiding this comment

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

I'm not super experienced with the global variables used in the test, but I do have a comment and a question

  1. It looks like the tests aren't running as part of any checks - @JustinBeckwith is this something we should change?

  2. When using global variables, is it truly possible for these tests to run independently of each other? (answer might be yes, but I just want to double check)

@JustinBeckwith
Copy link

@leahecole I'm going to be honest here, I did not know this repository existed. The fact that tests aren't run in CI seems bad, yeah 😆

Seems like a great opportunity to use GitHub Actions since they're a lot easier to set up and use than Kokoro, and this suite probably doesn't require any private keys or such.

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Apr 6, 2020

It looks like the tests aren't running as part of any checks. Is this something we should change?

Definitely yes. I'm ok with either setting up CI in this PR, or in another PR.

When using global variables, is it truly possible for these tests to run independently of each other? (answer might be yes, but I just want to double check)

We need to use global variables because we're testing a decorator around the test themselves. A single variable is used only in one single test. The name of the variable matches the name of the test.

@JustinBeckwith
Copy link

Actually thinking more about this:

  1. I am totally ok making this fix, landing it, and then unblocking your work here @tmatsuo
  2. @leahecole / @busunkim96 - could I trouble y'all to work together to give this repository the yoshi hug? Specifically looking for:
  • Using the consistent library CI and release process
  • Making sure it's registered in sloth and devrel/repos

@tmatsuo
Copy link
Contributor Author

tmatsuo commented Apr 6, 2020

@JustinBeckwith Thanks! To land this module in pypi, we need to be a maintainer on this module on pypi.

@theacodes @andrewsg
Can you make me an admin of this module? My username on pypi is also tmatsuo.

@leahecole leahecole mentioned this pull request Apr 6, 2020
@leahecole
Copy link
Contributor

SG. I set up time with Bu Sun for Wednesday @JustinBeckwith .

@leahecole leahecole merged commit f92cb05 into GoogleCloudPlatform:master Apr 6, 2020
@tmatsuo tmatsuo deleted the fix-eventually-consistent branch April 6, 2020 19:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

can not pass arguments to @eventually_consistent.call
3 participants