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

Feature/database as storage #989

Open
wants to merge 2 commits into
base: master
from

Conversation

@ClayLambertExp
Copy link

@ClayLambertExp ClayLambertExp commented Oct 22, 2020

Implementation on behalf of QOMPLX Inc, for providing database as a storage option for Polynote notebooks. All notebook CRUD operations operate against the database as if it were a file system. Thus all behavior matches that which is provided for File Systems. Whereby notebook data is stored in a database and may be shareable amongst multiple users.

This implementation is based on Postgres as the database store.

@jeremyrsmith
Copy link
Contributor

@jeremyrsmith jeremyrsmith commented Oct 23, 2020

@ClayLambertExp This is a great option and thanks for implementing it! I think this sort of thing would have to be a plugin, though. Do you mind if we keep this PR open for a bit while we figure out if the plugin infrastructure has enough hooks to do this sort of thing?

*
* Author: Clay Lambert, additions by Ken Hawley
* */
class DatabaseFilesystem(databaseContext: Postgres, maxDepth: Int = 4) extends NotebookFilesystem {

This comment has been minimized.

@Milyardo

Milyardo Oct 23, 2020

This class should be final

@angadsalaria
Copy link

@angadsalaria angadsalaria commented Oct 23, 2020

@ClayLambertExp This is a great option and thanks for implementing it! I think this sort of thing would have to be a plugin, though. Do you mind if we keep this PR open for a bit while we figure out if the plugin infrastructure has enough hooks to do this sort of thing?

@jeremyrsmith - Agree on the plugin based modular approach. Our team had same assessment. But wanted to leave to the community on how best to approach it. We'll be leaving the PR as is at this time, and have to move to other endeavors. Happy for you to keep the PR open for various contributors to align & time it with plugin hooks.

* */
class DatabaseFilesystem(databaseContext: Postgres, maxDepth: Int = 4) extends NotebookFilesystem {

var dbContext: DbContext = null

This comment has been minimized.

@Milyardo

Milyardo Oct 23, 2020

these should be private, also should probably be zio.Ref

* Returns the notebook content from the database wrapped in a Byte oriented stream.
* This makes a synchronous Quill call
*/
private def getNotebookContentStream(path: String): ByteArrayInputStream = {

This comment has been minimized.

@Milyardo

Milyardo Oct 23, 2020

Should suspend the side effects here inside ZIO

/**
* Localized database access functions. Here is where the actual database calls are made.
*/
class Database(dbContext: DbContext) {

This comment has been minimized.

@Milyardo

Milyardo Oct 23, 2020

If you suspend all of the database effects here in this class, you can ensure they're all captured in the filesystem.

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.