Codenvy-1127 docker instance implementation that that limits number of simultaneous container commits on the same node. #1166
Conversation
| semaphore = newSemaphore; | ||
| } | ||
| } | ||
| System.out.println("~~~~~~~~~~~~" + semaphore); |
…snapshots on same node.
mshaposhnik
changed the title
mshaposhnik
changed the title
| nodeSemaphore.acquire(); | ||
| super.commitContainer(repository, tag); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
Need to throw smthng here, but only IO is possible. WDYT ?
There was a problem hiding this comment.
throw new IOException(e) ?
…urrent snapshots on same node.
| executor.execute(() -> performCommit(repo3, TAG)); | ||
|
|
||
| // thread #3 should wait - semaphore is red | ||
| verify(dockerConnectorMock, never()).commit(Matchers.argThat(new CommitParamsMatcher(repo3))); |
There was a problem hiding this comment.
I think that timeout is needed here, otherwise there is no guarantee that 3rd thread was started. Also it would be nice to verify that Instance.commit is called
| executor.execute(() -> performCommit(repo1, TAG)); | ||
| executor.execute(() -> performCommit(repo2, TAG)); | ||
| waitingAnswer1.waitAnswerCall(1, TimeUnit.SECONDS); | ||
| waitingAnswer2.waitAnswerCall(1, TimeUnit.SECONDS); |
There was a problem hiding this comment.
repo2 also won't commit until repo1 commit is finished. Maybe remove it?
There was a problem hiding this comment.
It will - i set permits to 2
There was a problem hiding this comment.
Oh, sorry, I missed this
|
|
||
|
|
||
| @Test | ||
| public void shouldBeAbleToCommitSimultaneously() throws Exception { |
There was a problem hiding this comment.
Please add test for different nodes
…of concurrent snapshots on same node.
| super.commitContainer(repository, tag); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| throw new IOException(e); |
There was a problem hiding this comment.
new IOException(e, e.getLocalizedMessage());?
| */ | ||
| public class HostedDockerInstance extends DockerInstance { | ||
|
|
||
| private static final Map<String, Semaphore> SEMAPHORES = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
I wouldn't say that CONSTANT_CASE is the right one for the mutable value, see examples
… Avoid of concurrent snapshots on same node.
| Thread.sleep(200); //to allow thread # 3 start | ||
|
|
||
| // thread #3 commit executed too | ||
| verify(dockerInstance).commitContainer(eq(repo3), eq(TAG)); |
There was a problem hiding this comment.
You can use mockito times instead of Thread.sleep here, it can boost test
…Y-1127; Avoid of concurrent snapshots on same node.
What does this PR do?
Adds docker instance implementation that limits number of simultaneous container commits on the same node at the same time.
What issues does this PR fix or reference?
Fixes #1127
Previous Behavior
Multiple container commits may be possible on same node.
New Behavior
Commits are one-by-one
Tests written?
Docs requirements?
None