-
-
Notifications
You must be signed in to change notification settings - Fork 362
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
Reduces frontend caching, fixes a few bugs #3897
Conversation
…a, remove abstract_types from common data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really great. I like how the changes in stores/*
simplify a lot of things!
Instead of requesting changes, I just made them myself, and I'll kick this back to you to look over before merging. If you don't like my changes, we can continue discussing. Or if you're cool with this, then feel free to merge.
There was some naming I didn't like, so I pushed 2c39ff9 to adjust it.
Those naming changes also led me to dig deeper into AsyncRpcApiStore.runBatched
because I especially didn't like using the word "optimal" to refer to the logic of running something only when it's not initialized. The word "optimal" made me think it would always run but that it would do so using some special heuristics like compression or something. I think "conservative" is a better word (though not perfect) to indicate that it will happen only when necessary.
Also, we don't currently have a need for AsyncRpcApiStore
to handle arrays of BatchRunners with different modes. If we did have such a need, then passing an array of runners alongside an array of options would seem messy and hard to read because the options would not be lined up directly with their runners. This is the sort of code that I would not love:
AsyncRpcApiStore.runBatch(
[
foo.batchRunner({ a: 1 }),
bar.batchRunner({ b: 2 }),
],
{ mode: [ 'force-run', 'optimal' ] }
);
If we do eventually have a need for one batch with different modes like that, then I'd rather make some minor modifications to allow calling code that would look something like this:
AsyncRpcApiStore.runBatch([
foo.batchRunner({ a: 1 }),
bar.conservativeBatchRunner({ b: 2 }),
]);
But we don't even have that need yet, so I think it's best to err on the side of simplicity and only support batching via homogeneous modes.
Thanks @seancolsen! I think the name changes make sense. I added two small commits.
I'm going to merge this PR in since the commits are fairly minor. Please feel free to take a look and review retroactively. |
Fixes #3864
Fixes #3877
Reduces frontend caching: Only caches current schema, tables, and explorations
Fixes duplicate requests for
databases.get
Fixes common data: Null scenarios, removes abstract types
Fixes some minor translation issues (see commits)
Checklist
Update index.md
).develop
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin