Skip to content

Fix for issue#2742 : NullPointerException in disk store under high load on clear()#3114

Merged
chrisdennis merged 2 commits intoehcache:masterfrom
ajaymaddi31:issue-2742-new
Jan 23, 2023
Merged

Fix for issue#2742 : NullPointerException in disk store under high load on clear()#3114
chrisdennis merged 2 commits intoehcache:masterfrom
ajaymaddi31:issue-2742-new

Conversation

@ajaymaddi31
Copy link
Contributor

@ajaymaddi31 ajaymaddi31 commented Jan 19, 2023 •
edited
Loading

Fix for issue #2742 : NullPointerException in disk store under high load on clear()

chrisdennis
chrisdennis requested changes Jan 19, 2023
View reviewed changes
Copy link
Member

@chrisdennis chrisdennis left a comment

Choose a reason for hiding this comment

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

The changes look good... and I think the commit structure is legitimate. The merge commit needs to have a better commit message. It really needs to identify that it is a merge commit, and what it merges in to where, and why. So if you were merging main in to the fix branch then it would say that, if you merged the fix branch in to main (aka an integration branch) then it would mention that. Leaving the commit message as is would be confusing for anyone browsing the commit history in the future.

@jhouserizer
Copy link
Member

jhouserizer commented Jan 19, 2023

Leaving the commit message as is would be confusing for anyone browsing the commit history in the future.

I agree, because funny enough, it took me some effort to figure out what the difference was between the two commits.

@jhouserizer
Copy link
Member

jhouserizer commented Jan 19, 2023

Suggest: close the previous PR, with a comment pointing at this one as the replacement.

@ajaymaddi31 ajaymaddi31 mentioned this pull request Jan 20, 2023
Closed
@ajaymaddi31 ajaymaddi31 changed the title Issue 2742 new Fix for issue#2742 : NullPointerException in disk store under high load on clear() Jan 20, 2023
@ajaymaddi31 ajaymaddi31 requested review from chrisdennis and removed request for jhouserizer and kumarshilp January 23, 2023 11:04
chrisdennis
chrisdennis requested changes Jan 23, 2023
View reviewed changes
Copy link
Member

@chrisdennis chrisdennis left a comment

Choose a reason for hiding this comment

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

The changes are good, the commit structure is good, but the commit message on the merge commit is wrong. It doesn't merge f2c76ad into issue-2742 it appears it merges b7ffc81 in to the branch (or maybe the other way round, hard to tell from the GitHub UI).

@chrisdennis chrisdennis self-requested a review January 23, 2023 18:50
@chrisdennis chrisdennis merged commit cafa5bf into ehcache:master Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chrisdennis chrisdennis chrisdennis approved these changes

@jhouserizer jhouserizer Awaiting requested review from jhouserizer

@kumarshilp kumarshilp Awaiting requested review from kumarshilp

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ajaymaddi31 @jhouserizer @chrisdennis