search

sbtourist / Journal.IO

Journal.IO is a zero-dependency, fast and easy-to-use journal storage implementation.
Apache License 2.0
260 stars 39 forks source link

Invalid hints remain after compaction #52

Closed bseibel closed 10 years ago

bseibel commented 10 years ago

I had been seeing some random IllegalStateExceptions from hasRecordHeader. I was also able to get testConcurrentCompactionDuringWriteAndRead to fail the odd time after hammering on it repeatedly.

I'm not sure if there may be another underlying cause based on my limited understanding of the internals but it seems there are occasions where the hints map would contain old positions added from signalBatch after a compaction. This patch just ensures that only the hint added by compaction is present for the datafile just compacted.

sbtourist commented 10 years ago

Thanks for the submission, I'll have a look ASAP.

sbtourist commented 10 years ago

Just had a look.

It is right we miss to remove hints for compacted files, but I'd hold on this pull request because I think there are other small holes in the "post-compaction" behaviour, like when iterating over a completely compacted away data file, and I'd like to address those ones too: what do you think? Are you ok with that?

bseibel commented 10 years ago

Yeah for sure, I suspected as much since it didn't seem like this case should be possible.

-brs

On Dec 20, 2013, at 3:33 PM, Sergio Bossa notifications@github.com wrote:

Just had a look.

It is right we miss to remove hints for compacted files, but I'd hold on this pull request because I think there are other small holes in the "post-compaction" behaviour, like when iterating over a completely compacted away data file, and I'd like to address those ones too: what do you think? Are you ok with that?

— Reply to this email directly or view it on GitHubhttps://github.com/sbtourist/Journal.IO/pull/52#issuecomment-31039516 .

sbtourist commented 10 years ago

Hey @bseibel, I pushed some changes in the "issue52" branch: basically, I removed the "global" pause/resume, as it should be ok to only pause at single file compaction time, and used the already existent removeDataFile method to clear hints.

What do you think? Do you mind having a quick review?

sbtourist commented 10 years ago

I had to force-push a few times, so just refer to the "issue52" branch.

sbtourist commented 10 years ago

Also fixed iteration over compacted data file.

bseibel commented 10 years ago

Aha! I completely missed that removeDataFile cleans up hints; One goes blind after trying to figure out new code for awhile :). I'll rerun a few of my tests as soon as I get a free moment to see if it takes care of my use case too.

sbtourist commented 10 years ago

Cool, thanks much.

I'll be releasing 1.4.2 later today, just not sure if to include this issue...

bseibel commented 10 years ago

Haven't dug into why yet but about 1 in 40'ish runs of the Concurrent tests will cause testConcurrentCompactionDuringWriteAndRead to fail at assertEquals(iterations - deletions, locations); with locations being between 1-5 shy from the expected 900.

sbtourist commented 10 years ago

That's something I should have fixed in: https://github.com/sbtourist/Journal.IO/commit/991c1dd9d9841fae567941eead463d803f80095c

Do you have that commit?

bseibel commented 10 years ago

Yeah, this was out of your fork/issue52 branch.

sbtourist commented 10 years ago

@bseibel, that should be fixed now. I also improved tests a bit, hopefully no more bugs :)

bseibel commented 10 years ago

Looks good now! Thanks @sbtourist !

sbtourist commented 10 years ago

Thanks!