-
Notifications
You must be signed in to change notification settings - Fork 1.5k
chore: Update api docs for SessionContext
, TaskContext
, etc
#6106
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
Conversation
@@ -15,30 +15,7 @@ | |||
// specific language governing permissions and limitations | |||
// under the License. | |||
|
|||
//! This module contains the shared state available at different parts |
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 was hard to find, so I moved the content on to the structs that were referenced
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.
Nice work 👍
@@ -33,6 +33,15 @@ use url::Url; | |||
|
|||
#[derive(Clone)] | |||
/// Execution runtime environment. | |||
/// | |||
/// A [`RuntimeEnv`] can be created from a [`RuntimeConfig`] and | |||
/// stores state to be shared across multiple sessions. In most |
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.
Probably I'm wrong here, but not sure RuntimeEnv is shared between sessions.
The runtime will be shared between queries within single session.
But SessionContext::new()
creates new session with new UUID and new runtime.
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 a good point -- by default a SessionContext
creates a new instance of RuntimeEnv
-- however that means any limits are not enforced across queries. I think this comment was trying to say if you want limits enforced across multiple session contexts you need to use the same RuntimeEnv
I have tried to clarify this in 0bbf687
/// `RuntimeEnv`, and therefore will not enforce memory or disk | ||
/// limits for queries run on different `SessionContext`s. | ||
/// | ||
/// To enforce resource limits (e.g. to limit the total amount of |
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.
great, thanks
Which issue does this PR close?
Related to #3058
Rationale for this change
I am trying to make the API docs published to crates.io more useful and stand on their own
What changes are included in this PR?
SessionContext
and consolidate some other docs from theexecution
module that were hard to discoverAre these changes tested?
Yes (with CI checks)
Are there any user-facing changes?
Hopefully better docs