Skip to content

KAFKA-14767: Fix missing commitId build error after git gc#13315

Merged
ijuma merged 7 commits intoapache:trunkfrom
gharris1727:kafka-14767-missing-commit-id
Oct 22, 2023
Merged

KAFKA-14767: Fix missing commitId build error after git gc#13315
ijuma merged 7 commits intoapache:trunkfrom
gharris1727:kafka-14767-missing-commit-id

Conversation

@gharris1727
Copy link
Copy Markdown
Contributor

git gc moves commit hashes from individual .git/refs/heads/ to .git/packed-refs which is not read by the determineCommitId function.

Replace the existing lookup within the .git directory with a GrGit lookup that handles packed and unpacked refs transparently.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

Signed-off-by: Greg Harris <greg.harris@aiven.io>
@C0urante C0urante requested a review from ijuma February 28, 2023 14:29
Comment thread build.gradle Outdated
headRef.trim().take(takeFromHash)
}
} else if (file("$rootDir/.git").exists()) {
def repo = Grgit.open(currentDir: project.getRootDir())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The rat configuration does something similar, maybe we should have a variable for the git repo that can be used by both tasks.

Signed-off-by: Greg Harris <greg.harris@aiven.io>
@gharris1727 gharris1727 requested a review from ijuma February 28, 2023 23:16
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Feb 28, 2023

Looks like the build failed.

Signed-off-by: Greg Harris <greg.harris@aiven.io>
@gharris1727
Copy link
Copy Markdown
Contributor Author

@ijuma I don't know how my change would impact the execution order of rat vs processTestMessages, do you have any intuition?

The commitId change that I originally proposed (without the deduplication of the Grgit call) didn't experience this failure; do you think we could merge the commitId improvement and follow up on the rat implicit dependencies in another PR if the latest commit doesn't fix the build?

@gharris1727
Copy link
Copy Markdown
Contributor Author

It appears that the mustRunAfter directive has fixed the build, and now it's just normal flakey tests. I won't be reverting the fixups and should be ready for review.

@github-actions
Copy link
Copy Markdown

This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch)

@github-actions github-actions Bot added the stale Stale PRs label Jun 10, 2023
@gharris1727
Copy link
Copy Markdown
Contributor Author

@ijuma I'd still like to get this merged, please take another review pass, thanks!

@gharris1727 gharris1727 removed the stale Stale PRs label Jun 12, 2023
Signed-off-by: Greg Harris <greg.harris@aiven.io>
…g-commit-id

Signed-off-by: Greg Harris <greg.harris@aiven.io>
@gharris1727
Copy link
Copy Markdown
Contributor Author

The rat problem was fixed in #13854 so i'm removing my fix for it in this PR.

@github-actions
Copy link
Copy Markdown

This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch)

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

@github-actions github-actions Bot added the stale Stale PRs label Sep 22, 2023
@gharris1727
Copy link
Copy Markdown
Contributor Author

Hey @ijuma @divijvaidya @C0urante or @yashmayya Could you review this build improvement?

I'm still getting this git gc failure regularly when I try to build branches that have been idle for some time.
This also includes an improvement (the isDirectory guard) which allows us to use git worktree to check out and build multiple branches at one time.

I'd like to backport this to 3.4, 3.5, and 3.6.

Copy link
Copy Markdown
Member

@ijuma ijuma left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Oct 13, 2023

Well, Jenkins failed - we need to firs that first.

@github-actions github-actions Bot removed the stale Stale PRs label Oct 14, 2023
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Oct 22, 2023

Build looks good, merging.

@ijuma ijuma merged commit ffcb6d4 into apache:trunk Oct 22, 2023
gharris1727 added a commit that referenced this pull request Oct 23, 2023
git gc moves commit hashes from individual .git/refs/heads/ to .git/packed-refs which is not read
by the determineCommitId function.

Replace the existing lookup within the .git directory with a GrGit lookup that handles packed and
unpacked refs transparently.

Reviewers: Ismael Juma <ismael@juma.me.uk>
gharris1727 added a commit that referenced this pull request Oct 23, 2023
git gc moves commit hashes from individual .git/refs/heads/ to .git/packed-refs which is not read
by the determineCommitId function.

Replace the existing lookup within the .git directory with a GrGit lookup that handles packed and
unpacked refs transparently.

Reviewers: Ismael Juma <ismael@juma.me.uk>
gharris1727 added a commit that referenced this pull request Oct 23, 2023
git gc moves commit hashes from individual .git/refs/heads/ to .git/packed-refs which is not read
by the determineCommitId function.

Replace the existing lookup within the .git directory with a GrGit lookup that handles packed and
unpacked refs transparently.

Reviewers: Ismael Juma <ismael@juma.me.uk>
mjsax pushed a commit to confluentinc/kafka that referenced this pull request Nov 22, 2023
)

git gc moves commit hashes from individual .git/refs/heads/ to .git/packed-refs which is not read
by the determineCommitId function.

Replace the existing lookup within the .git directory with a GrGit lookup that handles packed and
unpacked refs transparently.

Reviewers: Ismael Juma <ismael@juma.me.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants