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

Provide a way to remove old commits #619

Closed
wants to merge 13 commits into from
Closed

Conversation

minwoox
Copy link
Member

@minwoox minwoox commented Jul 16, 2021

Motivation:
Central Dogma uses jGit to store data. Due to the nature of Git that stores unlimited history,
Central Dogma will eventually get in trouble managing disk usage.
We can handle this by maintaining the primary and secondary Git repositories internally.
This works in this way:

  • Commits are pushed to the primary Git repository.
  • If the number of commits exceeds the threshold (minRetentionCommits), then the secondary Git repository is created.
  • Commits are pushed to both primary and secondary Git repositories.
  • If the secondary Git repository has the number of commits more than the threshold;
    • The secondary Git repository is promoted to the primary Git repository.
    • The primary Git repository is removed completely.
    • Another secondary Git repository is created.
  • Back to the third.

Modifications:

  • Add CreateRollingRepositoryCommand that creates the rolling repository by the CommitRetentionManagementPlugin.
  • Add GitRepositoryV2 that manages the rolling jGit repositories.
    • The name of internal repositories will be: foo_0000000000, foo_0000000001, foo_0000000002 and so on
      • So that we can only store the suffix of the primary repo to maintain the repository. (i.e. We don't have to maintain the directory information of the secondary repo.)
      • RepositoryMetadataDatabase has the suffix in its file database.
    • GitRepository is not removed for the migration test.
  • Add InternalRepository that has jGit repository and CommitIdDatabase.
  • What happens if the revision of an operation(such as diff, watch, history, etc.) is lower than the firstRevision of the current primary repo?
    • If Revision.INIT(1) is used, the firstRevision is used instead.
      • e.g. diff(INIT, new Revision(100) ...) is equals to diff(new Revision(firstRevisionNumber), new Revision(100) ...)
    • If the Revision between Revision.INIT(1) and the firstRevision is used, a RevisionNotFoundException is raised.
      • except watch and findLatestRevision.

Result:

Todo:

  • Provide a way to set minRetentionCommits and minRetentionDay for each repository.
  • Unused internal repositories are removed by purging service.
  • Support mirroring from CD to external Git.

Motivation:
Central Dogma uses jGit to store data. Due to the nature of Git that stores unlimited history,
Central Dogma will eventually get in trouble managing disk usage.
We can handle this by maintaing the primary and secondary Git repository internally.
This works in this way:
1 Commits are pushed to the primary Git repository.
2 If the number of commits exceed the threshold (`minRetentionCommits`), then the secondary Git repository is created.
3 Commits are pushed to the both primary and secondary Git repositories.
4 If the secondary Git repository has the number of commits more than the threshold;
  - The secondary Git repository is promoted to the primary Git repository.
  - The primary Git repository is removed completely.
  - Another secondary Git repository is created.
5 Back to 3.

Modifications:
- TBD

Result:
- Close 575
- TBD

Todo:
- Provide a way to set `minRetentionCommits` and `minRetentionDay` for each repository.
- Support mirroring from CD to external Git.
@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #619 (958e7a9) into master (d596a34) will decrease coverage by 2.47%.
The diff coverage is 70.35%.

❗ Current head 958e7a9 differs from pull request most recent head a40a22d. Consider uploading reports for the commit a40a22d to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #619      +/-   ##
============================================
- Coverage     69.91%   67.43%   -2.48%     
- Complexity     3305     3434     +129     
============================================
  Files           333      340       +7     
  Lines         13135    14323    +1188     
  Branches       1427     1611     +184     
============================================
+ Hits           9183     9659     +476     
- Misses         3079     3727     +648     
- Partials        873      937      +64     
Impacted Files Coverage Δ
...ecorp/centraldogma/server/CentralDogmaBuilder.java 53.12% <0.00%> (-0.85%) ⬇️
...orp/centraldogma/server/CommitRetentionConfig.java 0.00% <0.00%> (ø)
.../linecorp/centraldogma/server/command/Command.java 74.35% <0.00%> (-13.52%) ⬇️
...server/command/CreateRollingRepositoryCommand.java 0.00% <0.00%> (ø)
...ogma/server/command/StandaloneCommandExecutor.java 83.87% <0.00%> (-3.53%) ⬇️
...internal/storage/DirectoryBasedStorageManager.java 63.31% <ø> (ø)
...ver/internal/storage/repository/CacheableCall.java 65.21% <ø> (ø)
...internal/storage/repository/RepositoryWrapper.java 28.57% <0.00%> (-2.68%) ⬇️
...rage/repository/git/CacheableCompareTreesCall.java 51.51% <ø> (ø)
...raldogma/server/storage/repository/Repository.java 72.11% <ø> (+9.18%) ⬆️
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d596a34...a40a22d. Read the comment docs.

@minwoox minwoox modified the milestones: 0.52.0, 0.52.1, 0.52.2 Aug 24, 2021
@minwoox minwoox marked this pull request as ready for review September 13, 2021 09:14
@minwoox
Copy link
Member Author

minwoox commented Sep 13, 2021

I think this is ready. Please, review this PR when you have time. 😄

Copy link
Member Author

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ikhoon All fixed. PTAL. 😉

@ikhoon ikhoon modified the milestones: 0.52.2, 0.53.0 Oct 8, 2021
@ikhoon ikhoon modified the milestones: 0.53.0, 0.54.0 Dec 3, 2021
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked through everything, but looks awesome! 👍 Let me keep taking a look, but only nits for now 🙏

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

class RepositoryMetadataDatabaseTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional; I think it might useful and trivial to add a test where we can verify RepositoryMetadataDatabase also reads the file correctly.

@Test
void writeAndRead() throws Exception {
    final RepositoryMetadataDatabase other = new RepositoryMetadataDatabase(
            db.getRootDir(), false);
    assertThat(other.primaryRepoDir().getName()).isEqualTo(db.primaryRepoDir().getName());
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the test. 🙇

if (!(firstRevision.major() <= revision.major() && revision.major() <= headRevision.major())) {
throw new RevisionNotFoundException(
"revision: " + revision +
" (expected: " + firstRevision.major() + " <= revision <= " + headRevision.major() + ")");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
" (expected: " + firstRevision.major() + " <= revision <= " + headRevision.major() + ")");
" (expected: " + firstRevision.major() + " <= revision <= " + headRevision.major() + ')');

@@ -185,23 +223,24 @@ private synchronized void put(Revision revision, ObjectId commitId, boolean safe
buf.flip();

// Append a record to the file.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Append a record to the file.
// Append or overwrite a record in the file.

requireNonNull(rollingRepositoryInitialRevision, "rollingRepositoryInitialRevision");
checkState(shouldCreateRollingRepository(rollingRepositoryInitialRevision,
minRetentionCommits, minRetentionDays) ==
rollingRepositoryInitialRevision, "aaa");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol 😝 aaa

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😱

// so we should catch up.
final RevisionRange revisionRange = new RevisionRange(
rollingRepositoryInitialRevision.forward(1), headRevision);
final List<Commit> commits = primaryRepo.listCommits(ALL_PATH, MAX_MAX_COMMITS,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q) I'm not sure how realistic/unrealistic the magic number 1000 is, but is it safe to say this won't be exceeded?
I'm asking because it seems like this implementation can break if secondaryRepo.HEAD != primaryRepo.HEAD (no problem if this isn't realistic)

I guess an alternative may be to allow partial secondaryRepo states.
This might also allow shorter write lock durations at the cost of additional complexity.

(or maybe we can just set an arbitrarily high value 🤔 )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I realized that we're only catching up revisions between the previous HEAD revision to the current HEAD revision. I guess this scenario isn't really realistic at least with the current specification 😅 feel free to ignore

Revision rollingRepositoryInitialRevision) {
writeLock();
try {
logger.info("Promoting the secondary repository in {}/{}.", parent.name(), originalRepoName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q) What do you think of validating secondaryRepo.HEAD == primaryRepo.HEAD here? (inside the write lock)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good suggestion. 👍

Comment on lines 532 to 534
* For example, when {@code minRetentionCommits} is set to 2000 and {@code minRetentionDays} is set to 14,
* the commits that are created more than 2000 are not removed until 14 days have passed. Set 0 to retain
* all commits.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused with the policy when both minRetentionCommits and minRetentionDays are set.

the commits that are created more than 2000 are not removed until 14 days have passed.

IIUC, if commits seem not to be removed in 14 days even if the number of commits exceeds 2000.
2000 minRetentionCommits with 14 minRetentionDays sounds like Central Dogma allows 2000 commits for 14 days.
If commits are created more than 2000 in 14 days, some old commits need to be removed. Although the old commits were less than 14 days old.

this.author = author;
}

private static boolean isEmpty(File dir) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Move this method below the constructors?

context.commandExecutor().execute(
Command.createRollingRepository(project.name(), repo.name(), revision,
config.minRetentionCommits(),
config.minRetentionDays()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call .join() for synchronous executions?

final ProjectManager pm = context.projectManager();
for (Project project : pm.list().values()) {
for (Repository repo : project.repos().list().values()) {
final Revision revision = repo.shouldCreateRollingRepository(config.minRetentionCommits(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check stopping for each repo so that a job successfully stops in gracefully shutting down time?

}
}

private GitRepositoryV2(Project parent, File repoDir, Executor repositoryWorker,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you leave a Javadoc or comment about the difference of the two constructors? :-)

}
}

static InternalRepository of(Project parent, String originalRepoName, File repoDir,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To rhyme with open?

Suggested change
static InternalRepository of(Project parent, String originalRepoName, File repoDir,
static InternalRepository create(Project parent, String originalRepoName, File repoDir,

throw new StorageException("found more than one parent: " +
gitRepo.getDirectory());
}
rebuild(gitRepo, revWalk, headRevision, revCommit, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findFirstRevisionOrRebuild()?

}
}

// Create a new instance only when necessary.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I guess most requests are sent with Revision.HEAD.

Revision headRevision = this.headRevision;
if (headRevision.major() == major) {
    return headRevision;
}

Comment on lines 756 to 760
if (secondaryRepo != null) {
promoteSecondaryRepo(secondaryRepo, rollingRepositoryInitialRevision);
} else {
createSecondaryRepo(rollingRepositoryInitialRevision);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per SPR, how about removing createSecondaryRepo() in promoteSecondaryRepo()?

Suggested change
if (secondaryRepo != null) {
promoteSecondaryRepo(secondaryRepo, rollingRepositoryInitialRevision);
} else {
createSecondaryRepo(rollingRepositoryInitialRevision);
}
if (secondaryRepo != null) {
promoteSecondaryRepo(secondaryRepo, rollingRepositoryInitialRevision);
}
createSecondaryRepo(rollingRepositoryInitialRevision);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good suggestion. 👍

@ks-yim
Copy link

ks-yim commented Jan 9, 2022

I am just asking out of curiosity 😅.

I thought of a very naive approach of doing something jGit equivalent for git rebase -i HEAD~N to rewrite commit history via fixup or squash when I first looked at the issue.

Rolling repository is awesome but at the same time looks relatively complicated compared to the rebase option.
What would be the benefit of rolling repo strategy assuming that we can do the same thing with rebase?

Maybe rewriting thousands of commit history could be too heavy or rebase doesn't give us a desired result at all?

@jrhee17
Copy link
Contributor

jrhee17 commented Jan 11, 2022

@minwoox is away for a while, so leaving my guess/analysis 😅 Feel free to correct me

I thought of a very naive approach of doing something jGit equivalent for git rebase -i HEAD~N to rewrite commit history via fixup or squash when I first looked at the issue.

From what I understand, this is what we're trying to do. We're trying to squash commits between INIT~HEAD^N.

Rolling repository is awesome but at the same time looks relatively complicated compared to the rebase option.
What would be the benefit of rolling repo strategy assuming that we can do the same thing with rebase?

My guess is so that we can make the switch atomically. If any of the intermediate steps fails while squashing (which I think would involve multiple IOs to disk) the repository may result in a corrupt state.


final InternalRepository repo = secondaryRepo != null ? secondaryRepo : primaryRepo;
if (exceedsMinRetention(repo, headRevision, minRetentionCommits, minRetentionDays)) {
return headRevision;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The {@link Repository} retains at least the number of {@code minRetentionCommits} when more than
* {@code minRetentionCommits} are made.

Q) From the javadocs, I guessed minRetentionCommits will be retained, but it seems like all commits up to headRevision will be squashed. Is my understanding correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it seems like all commits up to headRevision will be squashed

The returned headRevision will be used as the initial revision for creating the secondary repository. So it's not squashed. Did this what you mean?

author, summary, detail, markup, applyingChanges, false);
this.headRevision = res.revision;
final InternalRepository secondaryRepo = this.secondaryRepo;
if (secondaryRepo != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think it's possible that primaryRepo.HEAD can diverge from secondaryRepo.HEAD since the two calls to commit aren't transactional.
What do you think of adding the following condition to prevent further divergence?

Suggested change
if (secondaryRepo != null) {
if (secondaryRepo != null && Objects.equals(primaryRepo.headRevision(), secondaryRepo.headRevision())) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the two calls to commit aren't transactional.

We acquired writeLock before committing so I think it's transactional and it shouldn't diverge.
Let me add an assertion for that. 😉

// so we should catch up.
final RevisionRange revisionRange = new RevisionRange(
rollingRepositoryInitialRevision.forward(1), headRevision);
final List<Commit> commits = primaryRepo.listCommits(ALL_PATH, MAX_MAX_COMMITS,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I realized that we're only catching up revisions between the previous HEAD revision to the current HEAD revision. I guess this scenario isn't really realistic at least with the current specification 😅 feel free to ignore

@ks-yim
Copy link

ks-yim commented Jan 14, 2022

@jrhee17 Thanks for the reply!!

I thought we could just do something like..:

Git.wrap(jGitRepository).rebase().setUpstream("HEAD~2000").runInteractively(new InteractiveHandler() {
    @Override
    public void prepareSteps(List<RebaseTodoLine> steps) {
        if (steps.size() <= 2000) {
            return;
        }
        for (int i = 0; i < 1000; i++) {
            try {
                steps.get(i).setAction(Action.SQUASH);
            } catch (IllegalTodoFileModification e) {
                // exception handling..
            }
        }
    }

    @Override
    public String modifyCommitMessage(String commit) {
        return commit;
    }
})

My guess is so that we can make the switch atomically. If any of the intermediate steps fails while squashing (which I think would involve multiple IOs to disk) the repository may result in a corrupt state.

I am not sure if I am getting it correctly, but agree that there seems to be no easy way to handle IO errors or merge conflict with the rebase option (though I doubt squashing will cause conflict).

@ikhoon
Copy link
Contributor

ikhoon commented Jan 18, 2022

Git.wrap(jGitRepository).rebase().setUpstream("HEAD~2000")

If a repository has a long history, it would not release a write lock for a long time. The long lock causes to block threads waiting for the lock.
So we decided to avoid manipulating the working repository.
We tried to apply Git GC periodically, but it was reverted. #615
There is a mega repo in Central Dogma of LINE that has over 380k commits.
IIRC, it took almost 10 minutes to run Git GC on the repository.

@minwoox
Copy link
Member Author

minwoox commented Jan 19, 2022

That's a good question, @ks-yim and Thanks @jrhee17 and @ikhoon for the answer. 😄

There's another reason that I didn't use squash because it modifies the Git Hashes that we are internally using for revision mapping.
Let's look at the following example:

1th commit(hash: foo) -> 2nd commit(hash: bar) -> 3rd commit(hash: baz)

If we squash 1th and 2nd then the commits will be:

1th and 2nd commit(hash: fooX) -> 3rd commit(hash: bazX)
                                             This one is also changed.

The hashes of all commits after the squash point are changed so I can't just do the squash. 😄

@ks-yim
Copy link

ks-yim commented Jan 19, 2022

Thanks for the detailed comments!!
There's a lot of heavy lifting done for users under the hood in CD 👍!

@ikhoon ikhoon modified the milestones: 0.54.0, 0.55.0 Jan 28, 2022
@minwoox
Copy link
Member Author

minwoox commented Feb 23, 2022

I'm a bit confused with the policy when both minRetentionCommits and minRetentionDays are set.

Had a chat with @ikhoon and we decided to keep the current behavior. 😄

Copy link
Member Author

@minwoox minwoox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Address all comments, PTAL. 🙇

import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

class RepositoryMetadataDatabaseTest {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the test. 🙇

author, summary, detail, markup, applyingChanges, false);
this.headRevision = res.revision;
final InternalRepository secondaryRepo = this.secondaryRepo;
if (secondaryRepo != null) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the two calls to commit aren't transactional.

We acquired writeLock before committing so I think it's transactional and it shouldn't diverge.
Let me add an assertion for that. 😉


final InternalRepository repo = secondaryRepo != null ? secondaryRepo : primaryRepo;
if (exceedsMinRetention(repo, headRevision, minRetentionCommits, minRetentionDays)) {
return headRevision;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it seems like all commits up to headRevision will be squashed

The returned headRevision will be used as the initial revision for creating the secondary repository. So it's not squashed. Did this what you mean?

requireNonNull(rollingRepositoryInitialRevision, "rollingRepositoryInitialRevision");
checkState(shouldCreateRollingRepository(rollingRepositoryInitialRevision,
minRetentionCommits, minRetentionDays) ==
rollingRepositoryInitialRevision, "aaa");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😱

Comment on lines 756 to 760
if (secondaryRepo != null) {
promoteSecondaryRepo(secondaryRepo, rollingRepositoryInitialRevision);
} else {
createSecondaryRepo(rollingRepositoryInitialRevision);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good suggestion. 👍

Revision rollingRepositoryInitialRevision) {
writeLock();
try {
logger.info("Promoting the secondary repository in {}/{}.", parent.name(), originalRepoName);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good suggestion. 👍

@minwoox minwoox modified the milestones: 0.55.0, 0.55.1 Feb 23, 2022
@minwoox
Copy link
Member Author

minwoox commented Feb 23, 2022

https://github.com/line/centraldogma/runs/5300517235?check_suite_focus=true#step:5:659

It seems like the cache of the build exists and isn't cleaned somehow. 😓
Let me close this PR and send it again.

@minwoox minwoox closed this Feb 23, 2022
@jrhee17 jrhee17 modified the milestones: 0.56.0, 0.56.1 Mar 29, 2022
@minwoox minwoox removed this from the 0.56.0 milestone Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a way to remove the old commits
4 participants