feat: store info in .remote file for remote shards to avoid extra S3 - #436
eguguchkin wants to merge 4 commits into
Conversation
🔴 Performance DegradationSome benchmarks have degraded compared to the previous run. Show table
|
a76acff to
ed1cec9
Compare
🔴 Performance DegradationSome benchmarks have degraded compared to the previous run. Show table
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #436 +/- ##
==========================================
- Coverage 71.30% 71.28% -0.02%
==========================================
Files 250 250
Lines 21783 21795 +12
==========================================
+ Hits 15532 15537 +5
- Misses 5095 5097 +2
- Partials 1156 1161 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
8bb796f to
ef2ec2d
Compare
af52944 to
23ae859
Compare
37cd32b to
b86554b
Compare
| // hasAllIndexFiles reports whether all 5 split index files are present. | ||
| func (m *fracManifest) hasAllIndexFiles() bool { | ||
| return m.hasInfo && m.hasToken && m.hasOffsets && m.hasID && m.hasLID | ||
| return m.hasInfo && m.hasToken && m.hasOffsets && m.hasID && m.hasLID || m.hasIndex |
There was a problem hiding this comment.
nit: maybe wrap all the new files with parens like this (m.hasInfo && m.hasToken && m.hasOffsets && m.hasID && m.hasLID) || m.hasIndex? i find this more readable
| g.Go(func() error { return u.Upload(gctx, f.idFile) }) | ||
| g.Go(func() error { return u.Upload(gctx, f.lidFile) }) | ||
| g.Go(func() error { | ||
| infoFile, err := os.Open(f.BaseFileName + consts.InfoFileSuffix) |
There was a problem hiding this comment.
can you please explain why we need to open file before upload it?
There was a problem hiding this comment.
Well, because previously we kept file descriptor open and the file descriptor itself was a part of frac.Sealed.
But not anymore. I am not sure I like it though -- like we deal with .info file only on startup and then we basically can forget about it.
| if f.offsetsFile == nil { | ||
| f.openFile( | ||
| consts.OffsetsFileSuffix, | ||
| func(file *os.File) { f.offsetsFile = file }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
You said that you wanted to remove assignment via callbacks.
| RemoteFractionSuffix = ".remote" | ||
| RemoteFractionSuffix = ".remote" | ||
| RemoteFractionTmpSuffix = "._remote" | ||
| RemoteFractionInfoSuffix = ".remote-info" |
There was a problem hiding this comment.
I suggest to use only two extensions:
._remote-- fraction is partially offloaded;.remote-- fraction is fully offloaded;
The action sequence will be the same:
- Create hardlink
._remoteto.info; - Offload data and
renamefile._remoteto.remote; - All other fraction files (except
.remotewill be dropped);
The only issue is with legacy .remote files which do not contain info block but it is pretty easy to resolve:
(1) size = stat(.remote)
(2) if size != 0 => return load(.remote);
(3) return fetch(.index) or fetch(.info);
In my opinion .remote-info is not necessary. And we already have to deal with .remote suffix anyway.
eguguchkin
changed the title
4 participants
Summary
Remote fractions used to keep only an empty
.remotemarker locally, so on everystartup seq-db had to talk to S3 to figure out what a fraction is: the loader ran an
Exists(.index)check per remote fraction to detect legacy format, and the fractionInfo was recovered from
.frac-cacheor downloaded from S3.This PR stores a copy of the fraction Info locally as a
<frac>.remote-infofile,created at offload time. Fractions offloaded with this version restore their Info
with zero S3 requests at startup.
Changes
frac.Sealed.Offload)frac.infois hard-linked tofrac._remote, all index files + docs._remoteis atomically renamed to.remote-info(durable rename + directory fsync)..remotemarker is now created viadurable touch + rename.
._remotefrom a failed attempt is safely reused on retry — a singletransient S3 error no longer blocks offloading of the fraction until restart.
fracmanager)Exists(.index)S3 check fromdiscover();legacy vs split format is now derived from
Info.BinaryDataVer(
frac.IsSingleIndex()), theisLegacyflag is gone from all constructors.._remotefiles (interrupted offload) are detected in the manifest and cleanedup; fraction stays sealed and is offloaded again.
with a clear
logger.Fatalinstead of a nil-pointer panic later.frac.Remote.loadInfo) — 3-level fallback:.remote-info(new, no S3).infoon S3 (older offloaded fractions).indexon S3 (oldest fractions)common.Info.InfoOnDisk(not persisted) soIndexOnDiskincludes the info filesize;
Remote.openDocs/openRemoteFilereworked with explicitmustExistcontract; durable fs helpers (
DurableHardLink,DurableRenameFile,DurableTouchFile);Offloadreturns onlyerror.Compatibility
.infois still uploaded as before..remotemarker) keep workingvia the S3 fallbacks in
loadInfo.Testing
fracmanager/loader_test.gocover discovery with.remote-infoand all fallback scenarios (empty
.remote+ cache legacy/new, legacy fast path).Known TODOs (follow-ups)
.remote-infoand cache (see Graceful degration when dealing with remote storages #92).Fixes #435