configure resultset size limit #1480
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeffreylovitz
force-pushed
the
limit-resultset-size
branch
2 times, most recently
from
01d1528 to
02d31e2
Compare
swilly22
force-pushed
the
limit-resultset-size
branch
from
9741a77 to
73eab00
Compare
jeffreylovitz
suggested changes
Dec 8, 2020
src/config.c
Outdated
Comment on lines
14
to
18
| #define CACHE_SIZE "CACHE_SIZE" // Config param, the size of each thread cache size, per graph. | ||
| #define THREAD_COUNT "THREAD_COUNT" // Config param, number of threads in thread pool | ||
| #define OMP_THREAD_COUNT "OMP_THREAD_COUNT" // Config param, max number of OpenMP threads | ||
| #define VKEY_MAX_ENTITY_COUNT "VKEY_MAX_ENTITY_COUNT" // Config param, max number of entities in each virtual key | ||
| #define MAINTAIN_TRANSPOSED_MATRICES "MAINTAIN_TRANSPOSED_MATRICES" // Whether the module should maintain transposed relationship matrices |
There was a problem hiding this comment.
Why delete the comments?
src/config.c
Outdated
Comment on lines
330
to
332
| //---------------------------------------------------------------------- | ||
| // OpenMP thread count | ||
| //---------------------------------------------------------------------- |
There was a problem hiding this comment.
These comments should be aligned with the case statements like cache size is.
src/config.h
Outdated
Comment on lines
30
to
31
| uint thread_pool_size; // Thread count for thread pool. | ||
| uint omp_thread_count; // Maximum number of OpenMP threads. |
src/commands/cmd_config.c
Outdated
| const char *config_name = RedisModule_StringPtrLen(argv[2], NULL); | ||
|
|
||
| // return entire configuration | ||
| if(config_name[0] == '*') { |
There was a problem hiding this comment.
I think an actual strcmp is better, as redis supports wildcard matches like *max.
src/commands/cmd_config.c
Outdated
Comment on lines
61
to
62
| size_t n_whitelist = 1; | ||
| Config_Option_Field whitelist[1] = { Config_RESULTSET_MAX_SIZE }; |
There was a problem hiding this comment.
I feel like a whitelist of run-time fields would be more appropriate in config.h.
src/config.c
Outdated
|
|
||
| default : | ||
| ASSERT("invalid option field" && false); | ||
| return false; |
src/config.c
Outdated
| uint *omp_nthreads = va_arg(ap, uint*); | ||
| va_end(ap); | ||
|
|
||
| if(omp_nthreads == NULL) return false; |
There was a problem hiding this comment.
These guards seem overly permissive to me. If any of these values are returned as NULL, that's an error and should be asserted on.
| //---------------------------------------------------------------------- | ||
|
|
||
| default : | ||
| ASSERT("invalid option field" && false); |
There was a problem hiding this comment.
This is a handled error.
src/config.c
Outdated
| //---------------------------------------------------------------------- | ||
|
|
||
| default : | ||
| ASSERT("invalid option field" && false); |
There was a problem hiding this comment.
This is a handled error.
jeffreylovitz
added a commit
that referenced
this pull request
Dec 8, 2020
* configure resultset size limit * WIP, refactored configuration * runtime config GET/SET * test resultset implicit limit * retrieve entire configuration with GRAPH.CONFIG GET * * Address PR comments Co-authored-by: Jeffrey Lovitz <[email protected]> (cherry picked from commit 40c2c33)
swilly22
added a commit
that referenced
this pull request
Dec 8, 2020
* configure resultset size limit (#1480) * configure resultset size limit * WIP, refactored configuration * runtime config GET/SET * test resultset implicit limit * retrieve entire configuration with GRAPH.CONFIG GET * * Address PR comments Co-authored-by: Jeffrey Lovitz <[email protected]> (cherry picked from commit 40c2c33) * bump version to 2.2.11 Co-authored-by: Roi Lipman <[email protected]>
pnxguide
pushed a commit
to CMU-SPEED/RedisGraph
that referenced
this pull request
Mar 22, 2023
* configure resultset size limit * WIP, refactored configuration * runtime config GET/SET * test resultset implicit limit * retrieve entire configuration with GRAPH.CONFIG GET * * Address PR comments Co-authored-by: Jeffrey Lovitz <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow for result-set size limit to be configured, default is unlimited.