Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

DOSS is creating unnecessary numbers of containers in the interim staging area #76

Closed
ato opened this Issue · 3 comments

1 participant

@ato
Owner

Shaun spotted this issue on snowy. There's a whole bunch of containers with a single file in each.

The excess containers aren't sealed so it's not the size condition that's wrong. On init the Area is supposed to find its first existing open container and continue packing into it:

https://github.com/nla/doss/blob/master/src/doss/local/Area.java#L30

I guess that must be failing. Perhaps it's the query:

https://github.com/nla/doss/blob/master/src/doss/local/Database.java#L93

I always assumed that would return the first match. But perhaps if there's multiple rows JDBI returns "null". That would cause the observed behaviour at least.

How it might be getting into that situation of more than one open container in the first place:

  • Amber creates a new LocalBlobStore instance for each session.
  • Two sessions are opened simultaneously.
  • Each tries to find an open container but all are sealed so they both create new containers.

Possible fix to make DOSS more robust?

SELECT MIN(container_id) FROM containers WHERE sealed = 0 AND AREA = :area;

Need to think about the implications of the race too. When I wrote LocalBlobStore I was imagining it being called from multiple threads, not multiple instances of LocalBlobStore with a single shared database as Amber's currently doing.

@ato ato added the bug label
@ato
Owner

@scoen I think we should move the opening of LocalBlobStore in amber up from the AmberSession level to the AmberDb class so the sessions all share the same blobstore.

Hmm, it also looks like amber's never closing the BlobStore which needs fixing as it will be another file descriptor leak.

@ato ato referenced this issue from a commit
@ato ato Test container bug #76 hypothesis
Hypothesis was disproven.
ca5965e
@ato
Owner

My hypothesis was disproven by ca5965e. Now I've got no idea what's causing it.

@ato ato closed this issue from a commit
@ato ato Reading an old container shouldn't switch the current writing contain…
…er. fix #76

Renamed container to currentContainer to reduce the likelihood
of reintroducing this mistake.
92f7b4e
@ato ato closed this in 92f7b4e
@ato
Owner

@scoen fix for this released as doss 0.23.8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.