Garbage Collector: Eliminate double slash in URL #3525
Merged
Conversation
This file contains 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
…ce end with a slash
…uffix as it was erroneous
Jonathan-Rosenberg
requested review from
arielshaqed,
itaiad200,
talSofer and
lynnro314
Jonathan-Rosenberg
added
include-changelog
Jonathan-Rosenberg
changed the title
arielshaqed
requested changes
Jun 20, 2022
clients/spark/core/src/main/scala/io/treeverse/clients/GarbageCollector.scala
Show resolved
Hide resolved
|
|
||
| spark.close() | ||
| } | ||
|
|
||
| private def concatToGCLogsPrefix(storageNameSpace: String, key: String): String = { | ||
| val strippedKey = key.stripPrefix("/") |
There was a problem hiding this comment.
Nit: Why do we need this? No call has a leading slash.
There was a problem hiding this comment.
The method is not aware of the calls, it sanitizes the input...
talSofer
reviewed
Jun 20, 2022
clients/spark/core/src/main/scala/io/treeverse/clients/GarbageCollector.scala
Show resolved
Hide resolved
talSofer
reviewed
Jun 21, 2022
Comment on lines
+22
to
+24
| configFileSuffixTemplate = "%s/retention/gc/rules/config.json" | ||
| addressesFilePrefixTemplate = "%s/retention/gc/addresses/" | ||
| commitsFileSuffixTemplate = "%s/retention/gc/commits/run_id=%s/commits.csv" |
There was a problem hiding this comment.
This part fixes another bug we found in the GC-Azure integration! double thanks @Jonathan-Rosenberg :)
Up until this fix, while trying to get gc rules we got the following error:
: invalid namespace
500 Internal Server Error
But removing the superfluous "/" fixed it for the following reason -
Before the fix, "/_lakefs/settings/retention/gc/rules/config.json" was parsed without returning an error to
lakeFS/pkg/block/azure/adapter.go
Line 91 in a21c0fc
arielshaqed
approved these changes
Jun 21, 2022
clients/spark/core/src/main/scala/io/treeverse/clients/GarbageCollector.scala
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Both the lakeFS server and the Garbage Collector client write to the underlying storage during the garbage collection process.
At times, and in both parts, it generates and writes to paths with double slashes.
For example:
s3://some-bucket/some-path//_lakefs/logs/gc/summary.This PR aims to fix this problem.
Problems:
/as a delimiter as the first character to theformatPathWithNamespacefunction that expects it to not include the delimiter as a prefix./prefix to the_lakefspath. That resulted in a double slash if the storage namespace ended with a slash.Mitigation:
formatPathWithNamespacefunction.How was this tested?
Ran the GC process with a previously double slashed generated path and made sure it changed its behavior to a single slash.
Clarification
Although the code itself generates a double slash path, the final entry, in S3, has no double slash in it.
Closes #2732