Open
Conversation
pkosiec
force-pushed
the
pkosiec/lakebase-plugin
branch
from
ed174de to
a17ec82
Compare
pkosiec
force-pushed
the
pkosiec/lakebase-plugin-2
branch
from
52c4a14 to
884ae23
Compare
pkosiec
force-pushed
the
pkosiec/lakebase-plugin
branch
from
a17ec82 to
20c9a3a
Compare
pkosiec
force-pushed
the
pkosiec/lakebase-plugin-2
branch
from
280e89e to
1e776a2
Compare
pkosiec
force-pushed
the
pkosiec/lakebase-plugin-2
branch
from
1e776a2 to
b2e77c6
Compare
Collaborator
MarioCadenas
left a comment
MarioCadenas
left a comment
There was a problem hiding this comment.
In general code LGTM, I'm not so sure about the onSetupMessage thing though, let's discuss that
Comment on lines
+59
to
+62
| - `POST /api/analytics/query/:query_key` | ||
| - `POST /api/analytics/users/me/query/:query_key` | ||
| - `GET /api/analytics/arrow-result/:jobId` | ||
| - `GET /api/analytics/users/me/arrow-result/:jobId` |
Collaborator
There was a problem hiding this comment.
not related to this pr, but I just realized this is outdated
Suggested change
| - `POST /api/analytics/query/:query_key` | |
| - `POST /api/analytics/users/me/query/:query_key` | |
| - `GET /api/analytics/arrow-result/:jobId` | |
| - `GET /api/analytics/users/me/arrow-result/:jobId` | |
| - `POST /api/analytics/query/:query_key` | |
| - `GET /api/analytics/arrow-result/:jobId` |
| "description": "SPDX license identifier", | ||
| "examples": ["Apache-2.0", "MIT"] | ||
| }, | ||
| "onSetupMessage": { |
Collaborator
There was a problem hiding this comment.
so if we have multiple plugins, with the onSetupMessage, then we will have multiple instructions printing? also, this seems like something needed just for the lakebase plugin and not for a general use case, what other use cases do you see?
Comment on lines
+51
to
+52
| export { LakebasePlugin } from "./plugins/lakebase"; | ||
| export { ServerPlugin } from "./plugins/server"; |
Collaborator
There was a problem hiding this comment.
why do we need to export these?
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR introduces a thin wrapper on top of the Lakebase driver, to ensure consistent behavior.
Resolves https://databricks.atlassian.net/browse/LKB-9544
Notes
Should we really name it as "Lakebase"? Victor suggested using name "Transaction" but it doesn't resonate with me.
On the other hand,
oltp,database,postgresdon't sound like better options thanlakebase🤔 We need to distinguish this plugin with theanalyticsplugin somehow.IMO the "Lakebase" name is the best what we can do right now, even though it is not consistent with
analyticsplugin. Let's see how the other plugins are named and then rename it - or, if you have any ideas, feel free to suggest them already.All PRs
apps initflow, remove empty files after template rendering cli#4549