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

Remove redundant dir_exists call in ContentsManager.new_untitled #719

Open
jhamet93 opened this issue Mar 12, 2022 · 1 comment
Open

Remove redundant dir_exists call in ContentsManager.new_untitled #719

jhamet93 opened this issue Mar 12, 2022 · 1 comment

Comments

@jhamet93
Copy link
Contributor

jhamet93 commented Mar 12, 2022

Problem

There is redundant ContentsManager.dir_exists in the POST API handlers for interactions with the ContentsManager. The handler calls ContentsManager.dir_exists and the ContentsManager.new_untitled calls ContentsManager.dir_exists when creating the new file as a result of this API call.

For implementations that lean on storing data in remote locations such as blob storage systems (GCS, S3, etc) or any custom implementation that involves hosting data not on disk, the duplicate invocations of these methods on POST have a performance impact. For example, at Twitter we persist internal user's data remotely so these calls add latency to the user experience of creating new files with the JupyterLab FileBrowser.

Developers do have the ability to negate this redundant call by overriding the method but this seems wasteful since it would essentially be a copy/paste of the current implementation sans the dir_exists call which most likely will result in code drift if changes are ever made to the upstream implementation.

Proposed Solution

  • Eliminate the ContentsManager.dir_exists call in the ContentsManager.new_untitled implementation in the base ContentsManager classes.
  • Add a ContentsManager.dir_exists call in the PUT handler to keep that behavior consistent

Alternative Solution

  • ContentsManager implementations should solely be in charge of these checks rather than the API handlers to avoid this potential duplication

Additional context

I am more than willing to implement this if it seems like a desired solution.

@welcome
Copy link

welcome bot commented Mar 12, 2022

Thank you for opening your first issue in this project! Engagement like this is essential for open source projects! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please try to follow the issue template as it helps other other community members to contribute more effectively.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also an intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

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

1 participant