feat: support refreshing vended credentials in worker #175
Conversation
|
Thanks – will review in AM |
| // Merge initial storage options (from namespace.describeTable) with base storage options | ||
| Map<String, String> merged = new java.util.HashMap<>(readOptions.getStorageOptions()); | ||
| merged.putAll(initialStorageOptions); | ||
| return new ReadOptions.Builder().setStorageOptions(merged).build(); |
There was a problem hiding this comment.
In a few places I noticed that only the storage options are set, does it matter that other read options are not cloned?
| val uuid = UUID.randomUUID() | ||
| val indexType = IndexTypeUtils.buildIndexType(method) | ||
|
|
||
| // Get namespace info from catalog if available (for credential vending on workers) |
There was a problem hiding this comment.
I see this logic is duplicated across OptimizeExec and AddExec. Can we make this a shared helper? I also wonder if it's easy to potentially forget to add this logic on new execs.
pom.xml
Outdated
| <lance.version>2.0.0-beta.3</lance.version> | ||
| <lance-namespace.version>0.3.1</lance-namespace.version> | ||
| <lance.version>2.0.0-beta.8</lance.version> | ||
| <lance-namespace.version>0.4.5</lance-namespace.version> |
There was a problem hiding this comment.
Maybe separate out the version changes unless we need core API changes?
| // Build read options with storage options provider for credential vending | ||
| val lanceReadOptions = (namespaceImpl, namespaceProperties, tableId) match { | ||
| case (Some(impl), Some(props), Some(tid)) => | ||
| val ns = LanceRuntime.getOrCreateNamespace(impl, props.asJava) |
There was a problem hiding this comment.
Seems like this logic could be extracted to a helper shared between both *Exec files and the DeltaWrite class? Ideally just want to avoid maintenance burden of updating namespace building logic everywhere.
| LanceSparkWriteOptions writeOptions, Map<String, String> initialStorageOptions) { | ||
| if (initialStorageOptions != null && !initialStorageOptions.isEmpty()) { | ||
| // Merge initial storage options with write options' storage options | ||
| Map<String, String> merged = new java.util.HashMap<>(writeOptions.getStorageOptions()); |
There was a problem hiding this comment.
nit: don't think java.util. prefix is required – I wonder if this could be added as a lint check maybe?
jackye1995
force-pushed
the
cred-vend-read-write
branch
from
cc301b5 to
d17cf88
Compare
jackye1995
force-pushed
the
cred-vend-read-write
branch
from
d17cf88 to
e1527ef
Compare
|
Merging this first to get other PRs going |
3 participants
Based on #174
Make the changes across the codebase so that we record namespace impl and properties in initial Spark catalog initialization time, and then pass it to worker to perform read and write to properly refresh vended credentials.
Also some refactoring to centralize global caches to
LanceRuntimecc @bryanck @jtuglu1