Skip to content
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

In-memory mode creates physical tables (in the current directory) #1374

Closed
damz opened this issue Jun 17, 2020 · 5 comments · Fixed by #1375
Closed

In-memory mode creates physical tables (in the current directory) #1374

damz opened this issue Jun 17, 2020 · 5 comments · Fixed by #1375
Labels
good first issue These are simple issues that can be picked up by new contributors kind/bug Something is broken. status/accepted We accept to investigate or work on it.

Comments

@damz
Copy link
Contributor

damz commented Jun 17, 2020

It looks like the in-memory mode creates physical tables in the current directory. This is because *DB.handleFlushTask does not seem to have a special case for the in-memory mode.

Steps to Reproduce the issue

  • Run go test . and watch SST files being created and destroyed in the current directory.
@yashkothari42
Copy link
Contributor

Hey @damz

I think this is happening because there isn't any check handling the case when InMemory is true but KeepL0InMemory is false. This causes badger to flush L0 on disks. This scenario is happening here.

badger/levels_test.go

Lines 772 to 779 in dd332b0

t.Run("with KeepL0InMemory", func(t *testing.T) {
opt.KeepL0InMemory = true
test(t, &opt)
})
t.Run("with L0 on disk", func(t *testing.T) {
opt.KeepL0InMemory = false
test(t, &opt)
})

@ashish-goswami, is this intended?

I guess there are two ways to handle it

  1. return Error while opening db if we encounter this case
  2. force KeepL0InMemory to be true when InMemory is true.

@damz
Copy link
Contributor Author

damz commented Jun 18, 2020

Indeed, the easiest fix would be to make this check db.opt.InMemory || db.opt.KeepL0InMemory:

badger/db.go

Lines 1023 to 1029 in dd332b0

if db.opt.KeepL0InMemory {
tbl, err := table.OpenInMemoryTable(tableData, fileID, &bopts)
if err != nil {
return errors.Wrapf(err, "failed to open table in memory")
}
return db.lc.addLevel0Table(tbl)
}

@jarifibrahim jarifibrahim added good first issue These are simple issues that can be picked up by new contributors kind/bug Something is broken. status/accepted We accept to investigate or work on it. labels Jun 18, 2020
@jarifibrahim
Copy link
Contributor

The code @damz pointed out is the correct place to fix this. This is a low hanging fruit and I'll leave it for new contributors.

Thank you @damz and @itsyashkothari !

@yashkothari42
Copy link
Contributor

yashkothari42 commented Jun 18, 2020

Hi @jarifibrahim

Wouldn't the solution provided result in inconsistency in logic.

For example here

badger/level_handler.go

Lines 191 to 195 in dd332b0

// Return false only if L0 is in memory and number of tables is more than number of
// ZeroTableStall. For on disk L0, we should just add the tables to the level.
if s.db.opt.KeepL0InMemory && len(s.tables) >= s.db.opt.NumLevelZeroTablesStall {
return false
}

KeepL0InMemory==false but actually L0 is in memory.

These are my proposed solutions -
1.

if !opt.KeepL0InMemory && opt.InMemory {
        return nil,errors.Errorf("Can not have KeepL0InMemory false and InMemory true.") 
}
opt.KeepL0InMemory = opt.KeepL0InMemory || opt.InMemory 

I would love to submit PR for this

@jarifibrahim
Copy link
Contributor

Approach 1 would mean the user has to set two options to enable in-memory mode. This doesn't seem like a good user experience.

opt.KeepL0InMemory = opt.KeepL0InMemory || opt.InMemory

This would result in the least amount of changes. Let's go with this approach.

jarifibrahim pushed a commit that referenced this issue Jun 19, 2020
InMemory mode would create L0 sst tables when KeepL0InMemory
was false as mentioned in #1374 This commit will keep L0 in
memory if either KeepL0InMemory is set or InMemory is set.

On running ` go test .`
Before: sst files were visible for a second 
Now: sst files won't created/visible

Fixes #1374
jarifibrahim pushed a commit that referenced this issue Oct 2, 2020
InMemory mode would create L0 sst tables when KeepL0InMemory
was false as mentioned in #1374 This commit will keep L0 in
memory if either KeepL0InMemory is set or InMemory is set.

On running ` go test .`
Before: sst files were visible for a second 
Now: sst files won't created/visible

Fixes #1374
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue These are simple issues that can be picked up by new contributors kind/bug Something is broken. status/accepted We accept to investigate or work on it.
Development

Successfully merging a pull request may close this issue.

3 participants