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

DEFAULT_LIMIT is not working #763

Open
tonyrosset opened this issue Jun 13, 2024 · 13 comments
Open

DEFAULT_LIMIT is not working #763

tonyrosset opened this issue Jun 13, 2024 · 13 comments
Assignees

Comments

@tonyrosset
Copy link

DEFAULT_LIMIT when set to a value more than 10000, it has noeffect it directs back to 10000

environment:
- CRATE_HOST=crate
- REDIS_HOST=redis
- REDIS_PORT=${REDIS_PORT}
- LOGLEVEL=ERROR
- DEFAULT_LIMIT=1000000

@c0c0n3
Copy link
Member

c0c0n3 commented Jun 14, 2024

@tonyrosset thanks for reporting this, we'll look into it...

@c0c0n3
Copy link
Member

c0c0n3 commented Jun 14, 2024

@tonyrosset are you using the limit and last_n params in the API call? If not, it could be there's a problem with CrateDB. I'm just looking at the code that figures out what the SQL limit actual value should be:

What's the API call you make to retrieve results?

@tonyrosset
Copy link
Author

thanks,
I am not using the limit or lastN. By default, when I set no DEFAULT_LIMIT I get 10,000 rows in the API call. When I set it to 100, I get 100 rows as supposed to be. The problem is only when I try to set the limit more than 10,000.

http://localhost:8668/v2/types/hwsensors?attrs=airTemperature&fromDate=2024-05-01T13:00:00&toDate=2024-06-12T09:00:00

@c0c0n3
Copy link
Member

c0c0n3 commented Jun 19, 2024

@tonyrosset, @pooja1pathak has just landed #765 which should fix the issue.

@tonyrosset
Copy link
Author

@pooja1pathak thanks,
@c0c0n3 Once u have accepted the commit at your repo I can use it, right?

@c0c0n3
Copy link
Member

c0c0n3 commented Jun 19, 2024

Once u have accepted the commit at your repo I can use it

if you only need to link the code, then the fix will be available as soon as I merge #765 into the main branch. But I guess you'd like to download a Docker image with the fix? We have a GitHub action to automatically build and push Docker images to DockerHub when a PR gets merged into main:

but unfortunately it's broken at the moment---I think it's just a silly thing w/ credentials having expired. Anyhoo, you can still build your own Docker image using the Docker file in the repo's root dir:

there's also a build script:

Hope that helps!

@tonyrosset
Copy link
Author

tonyrosset commented Jun 19, 2024

thanks, that helps,
also, it would be great if you let me know when the docker image is updated.
Thanks once again.

@c0c0n3
Copy link
Member

c0c0n3 commented Jun 19, 2024

sure no prob!

it would be great if you let me know when the docker image is updated.

If you subscribe to repo notifications, GitHub will send you an email every time we merge a PR...

@c0c0n3
Copy link
Member

c0c0n3 commented Jul 8, 2024

@tonyrosset #765 should've fixed the problem. So I'm closing this GitHub issue, but please feel free to reopen if the fix doesn't work for you.

@c0c0n3 c0c0n3 closed this as completed Jul 8, 2024
@c0c0n3 c0c0n3 reopened this Jul 8, 2024
@c0c0n3
Copy link
Member

c0c0n3 commented Jul 8, 2024

@tonyrosset sorry I've done this too quickly. We haven't merged #765 into main yet, so there's no easy way for you to test just yet. I'll close this issue when @pooja1pathak is done w/ #765 and we finally merge it. Sorry for the confusion!

@tonyrosset
Copy link
Author

It is okay. we can wait

@tonyrosset
Copy link
Author

Hi all, I'm not sure if you have merged it. Do let me know the status ) @c0c0n3 @pooja1pathak

@pooja1pathak
Copy link
Collaborator

@tonyrosset some tests are failing for the fix. I am working on it and will update once it gets fixed.

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

No branches or pull requests

3 participants