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

fix: async bucket_factory.get() #171

Merged
merged 4 commits into from
Apr 30, 2024
Merged

Conversation

haoyuhu
Copy link
Contributor

@haoyuhu haoyuhu commented Apr 28, 2024

Change-Id: Ib471e7661c7d9808ddad3767806b08f1ef1b9967

I have discovered an error that occurs at runtime when I use aioredis's asyncio redis_client to build the RedisBucket. The error message is "Invalid bucket: item: {name}".
I have attempted to fix this bug and would appreciate your assistance with the code review, thank you very much!

class ToolRuntimeMultiBucketFactory(BucketFactory):
    def __init__(self, clock: AbstractClock = TimeClock()):
        self.clock = clock
        self.buckets = {}
        self.thread_pool = None

    def wrap_item(self, runtime: ToolRuntimeParams, weight: int = 1) -> RateItem:
        """Time-stamping item, return a RateItem"""
        now = self.clock.now()
        return ToolRuntimeRateItem(runtime, now, weight=weight)

    async def get(self, item: ToolRuntimeRateItem) -> AbstractBucket:
        rates = _build_rates(item.runtime.quota_obj())
        if item.name not in self.buckets:
            # Use `self.create(..)` method to both initialize new bucket and calling `schedule_leak` on that bucket
            # We can create different buckets with different types/classes here as well
            new_bucket = await RedisBucket.init(rates, async_redis_client, f"rl_bucket:{item.name}")
            self.schedule_leak(new_bucket, self.clock)
            self.buckets.update({item.name: new_bucket})

        bucket: AbstractBucket = self.buckets[item.name]
        bucket.rates = rates
        return bucket

Change-Id: Ib471e7661c7d9808ddad3767806b08f1ef1b9967
@vutran1710
Copy link
Owner

the code basically looks good, but I think you will have to add some test for this async-get-bucket method.

You can modify the bucket-factory test to cover the case where get-bucket is async.

@vutran1710
Copy link
Owner

And use pre-commit to fix the lint complaints as well

@vutran1710
Copy link
Owner

Somehow the test suite did not pass :(. Can you check that? @haoyuhu

Change-Id: Ie61fe2e68cba6b44b46127772a18a43924746b3f
Change-Id: Ib66d223e070dddb42833812f884163a7cea0e7f4
@haoyuhu
Copy link
Contributor Author

haoyuhu commented Apr 30, 2024

Somehow the test suite did not pass :(. Can you check that? @haoyuhu

I have tried to fix the aforementioned issue, but the unit tests are not running properly on my local machine. Additionally, regarding the error message "Avoid too many return statements within this function", I don't have a better idea at the moment. Do you have any suggestions or recommendations?

@vutran1710
Copy link
Owner

Somehow the test suite did not pass :(. Can you check that? @haoyuhu

I have tried to fix the aforementioned issue, but the unit tests are not running properly on my local machine. Additionally, regarding the error message "Avoid too many return statements within this function", I don't have a better idea at the moment. Do you have any suggestions or recommendations?

You can ignore the "too many return" thing.

But if the tests are not running well then its probably due to pytest-asyncio & nox. Im afraid you have to play with it a bit to get it right.

Change-Id: Id0f0405febbc0d39acacadf35520b9e420b33f7e
@haoyuhu
Copy link
Contributor Author

haoyuhu commented Apr 30, 2024

I have fixed the issues with the unit tests, and I have also gone through the code and gained a deeper understanding. Please help me confirm if there are any areas that have been overlooked. Thank you!

tests/test_limiter.py Show resolved Hide resolved
@vutran1710
Copy link
Owner

Everything looks good.

@vutran1710 vutran1710 merged commit c6d5f88 into vutran1710:master Apr 30, 2024
4 of 5 checks passed
@haoyuhu
Copy link
Contributor Author

haoyuhu commented Apr 30, 2024

Everything looks good.

Can you help me create a new version? I'm looking forward to being able to download the latest version from PyPI. My code depends on this fix. Thank you.

@vutran1710
Copy link
Owner

Everything looks good.

Can you help me create a new version? I'm looking forward to being able to download the latest version from PyPI. My code depends on this fix. Thank you.

It's done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants