-
Notifications
You must be signed in to change notification settings - Fork 75
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
Add URI generator, resolver and corresponding document open & create functions #3211
Conversation
7d406eb
to
0f9d636
Compare
core/amber/src/main/scala/edu/uci/ics/texera/web/service/WorkflowService.scala
Outdated
Show resolved
Hide resolved
...flow-core/src/main/scala/edu/uci/ics/amber/core/storage/result/iceberg/IcebergDocument.scala
Show resolved
Hide resolved
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.
left minor comments.
92e7a04
to
136f9a6
Compare
f0bc26c
to
6d56d81
Compare
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.
Left other comments offline.
* @param operatorId Operator identifier. | ||
* @param portIdentity Optional port identifier. **Required** if `resourceType` is `RESULT` or `MATERIALIZED_RESULT`. | ||
* @return A VFS URI | ||
* @throws IllegalArgumentException if `resourceType` is `RESULT` but `portIdentity` is missing. |
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.
I suggest you use multiple explicit APIs instead:
createPortResultURI(workflowId, eId, opId, portId)
createOperatorLogURI(workflowId, eid, opId)
case Some(existingUris) => Some(uri :: existingUris) // Prepend URI to the existing list | ||
case None => Some(List(uri)) // Create a new list if key doesn't exist | ||
} | ||
} |
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.
don't we need to remove some URI?
@@ -188,7 +202,7 @@ class ExecutionResultService( | |||
2.seconds, | |||
resultPullingFrequency.seconds | |||
) { | |||
onResultUpdate(physicalPlan) | |||
onResultUpdate(executionId, physicalPlan) |
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.
Physical plan should have context already, which contains the Eid?
This PR refactor the storage key by representing it using the storage URI. And this PR also adds the
openDocument
andcreateDocument
function using the given URI.Major Changes
1. VFS URI resource type definition, resolve and decode functions
Two types of VFS resources are defined:
Two defs are added to the
FileResolver
resolve
: create the URI pointing to the storage resource on the VFSdecodeVFSUri
: decode a VFS URI to components2.
createDocument
andopenDocument
functions to theDocumentFactory
createDocument
andopenDocument
defs to create/open a storage resource pointed by the URIDocumentFactory.createDocument
DocumentFactory.openDocument