Don't check for existence twice if we know it is not a part file (objectstorage only)#31299
Don't check for existence twice if we know it is not a part file (objectstorage only)#31299mrow4a wants to merge 1 commit into
Conversation
|
These function was confusing (readFirstBlock), so better compare differences in the PhpStorm |
| * @param string|resource $path | ||
| * @return string | ||
| */ | ||
| protected function readFirstBlock($path) { |
There was a problem hiding this comment.
readFirstBlock was generalized for resource and storage -> I created dedicated functions, readResourceFirstBlock and readStorageFirstBlock.
There was a problem hiding this comment.
"readFirstBLock function" was also checking if path exists.. loosing 1 query per PUT.
| */ | ||
| protected function getHeader($path) { | ||
| $filecacheExists = false; | ||
| if (\is_resource($path)) { |
There was a problem hiding this comment.
Now I handle the "header" logic separately for resource and separately for storage.
| $firstBlock = $this->readStorageFirstBlock($realFile); | ||
| $filecacheExists = true; | ||
| $path = $realFile; | ||
| } else if ($realFile != $path && $this->storage->file_exists($path)) { |
There was a problem hiding this comment.
It makes no sense to ask again if $realFile==$path..
There was a problem hiding this comment.
Due to this fact, we are wining here 1 filecache query for each PUT. @DeepDiver1975 @PVince81 @butonic
| $realFile = $this->util->stripPartialFileExtension($path); | ||
| $targetExists = $this->file_exists($realFile) || $this->file_exists($path); | ||
| $targetExists = false; | ||
| if ($this->file_exists($realFile) || ($realFile != $path && $this->file_exists($path))) { |
There was a problem hiding this comment.
We are wining another 2 filecache queries here (file_exists function with objecstore needs 2 queries to be performed per call) @PVince81 @DeepDiver1975 @butonic
Codecov Report
@@ Coverage Diff @@
## master #31299 +/- ##
============================================
- Coverage 63.46% 63.46% -0.01%
- Complexity 18529 18534 +5
============================================
Files 1167 1167
Lines 69498 69505 +7
Branches 1264 1264
============================================
+ Hits 44108 44111 +3
- Misses 25021 25025 +4
Partials 369 369
Continue to review full report at Codecov.
|
|
conflicts |
|
Resolved conficts, and added description above how I tested it (including queires analysis) |
|
However, if this PR #28166 goes into live (cache in filecache), this PR is more of refactoring and cleaning the code. |
|
Can be obsoleted by #31486 |
|
I refresh this PR, since due to the fact there is no decision on introducing "cache of filecache", this PR is fixing the code logic. |
| $firstBlock = ''; | ||
| } | ||
| } | ||
| $firstBlock = $this->readFirstBlock($path); |
There was a problem hiding this comment.
Instead of using one generalized function, which introduced issue - I have two dedicated ones.
| $path = $realFile; | ||
| // Original file pointed by part file exists in the storage | ||
| $firstBlock = $this->readStorageFirstBlock($realFile); | ||
| } else if ($realFile != $path && $this->storage->file_exists($path)) { |
There was a problem hiding this comment.
Dont check if file_exists again, if realFile is the same as path (points to the same file)
Add unit tests for encryption readHeader
|
Most likely being closed with #31958 |
|
Obsoleted by #31958 |
3 participants
Description
Don't check for existence in the storage (e.g. objectstorage) twice if we know it is not a part file.
In that section of the code, we were checking for file existence in the storage twice for the same path. In case of part file we indeed need to check twice, but otherwise not.
There is no difference for Filesystem storage, but Objectstorage/Files_primary_s3 is affected.
How did I test