carloea2 commented on code in PR #4177:
URL: https://github.com/apache/texera/pull/4177#discussion_r2726210252
##########
file-service/src/main/scala/org/apache/texera/service/resource/DatasetResource.scala:
##########
@@ -384,11 +428,13 @@ class DatasetResource {
}
// Create a commit in LakeFS
- val commit = LakeFSStorageClient.createCommit(
- repoName = repositoryName,
- branch = "main",
- commitMessage = s"Created dataset version: $newVersionName"
- )
+ val commit = withLakeFSErrorHandling {
+ LakeFSStorageClient.createCommit(
+ repoName = repositoryName,
+ branch = "main",
+ commitMessage = s"Created dataset version: $newVersionName"
+ )
+ }
Review Comment:
**We can keep this approach for now.**
It would be good to open a discussion, and would be good if @Ma77Ball can
give his input since he is working in logging. Here is the things to discuss
later:
I think this PR is really about **how we want error handling to work across
the services calls** (LakeFS now, others later): do we *force* callers to deal
with failures, or do we centralize + make it easy/consistent.
### Option 1: change defs and force handling (typed)
Return `Either[StorageError, A]` (or `F[Either[...]]`). Caller can’t ignore
it.
```scala
storage.commit(repo, branch, msg): Either[StorageError, CommitId]
storage.uploadPart(key, part, bytes): Either[StorageError, Unit]
// usage
storage.commit(repo, branch, msg).map(Ok(_)).leftMap(mapToHttp)
```
Pros: type-safe + enforced. Cons: signature churn + lots of callsite updates
+ need `Error` mapping.
### Option 2: implicit + `Dynamic` `.safe` wrapper (ergonomic, but risky)
**This option adds automatic logs**
Nice callsite but reflection + weak typing (`Any`) + possible runtime method
lookup issues. Notice this method does not require any changes to the def like
commit, or uploadPart but it is optional to the caller to use `.safe` and
Changing the return type (2.1)
```scala
lakefs.safe.commit(repo, branch, msg) // Either[Throwable, Any]
datasetResource.safe.uploadPart(key, part, bs) // Either[Throwable, Any]
```
Keeping the return type and throwing (2.2)
```scala
lakefs.safe.commit(repo, branch, msg)
datasetResource.safe.uploadPart(key, part, bs)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
