You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "showuon (via GitHub)" <gi...@apache.org> on 2023/05/01 06:55:19 UTC
[GitHub] [kafka] showuon opened a new pull request, #13653: KAFKA-14946: fix NPE when merging the deltatable
showuon opened a new pull request, #13653:
URL: https://github.com/apache/kafka/pull/13653
Fix NPE while merging the deltatable. Because it's possible that hashTier is not null but deltatable is null (ex: removing data), we should have null check while merging for `deltatable` like other places did. Also added tests for it.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe merged pull request #13653: KAFKA-14946: fix NPE when merging the deltatable
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe merged PR #13653:
URL: https://github.com/apache/kafka/pull/13653
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #13653: KAFKA-14946: fix NPE when merging the deltatable
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #13653:
URL: https://github.com/apache/kafka/pull/13653#issuecomment-1529412878
@niket-goel @cmccabe @mumrah , please take a look. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on pull request #13653: KAFKA-14946: fix NPE when merging the deltatable
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on PR #13653:
URL: https://github.com/apache/kafka/pull/13653#issuecomment-1532350519
@cmccabe , PR updated and add tests. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a diff in pull request #13653: KAFKA-14946: fix NPE when merging the deltatable
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #13653:
URL: https://github.com/apache/kafka/pull/13653#discussion_r1183102365
##########
server-common/src/main/java/org/apache/kafka/timeline/SnapshottableHashTable.java:
##########
@@ -113,7 +113,7 @@ public void mergeFrom(long epoch, Delta source) {
HashTier<T> other = (HashTier<T>) source;
// As an optimization, the deltaTable might not exist for a new key
// as there is no previous value
- if (other.deltaTable != null) {
+ if (deltaTable != null && other.deltaTable != null) {
Review Comment:
Ah, you are right! We need to copy the other deltatable to `this`. Will update it later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on pull request #13653: KAFKA-14946: fix NPE when merging the deltatable
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on PR #13653:
URL: https://github.com/apache/kafka/pull/13653#issuecomment-1531861352
Thanks, this is a very good find, @showuon ! We've been seeing this NPE occassionally but never found out where it was coming from.
I left a correction, take a look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] cmccabe commented on a diff in pull request #13653: KAFKA-14946: fix NPE when merging the deltatable
Posted by "cmccabe (via GitHub)" <gi...@apache.org>.
cmccabe commented on code in PR #13653:
URL: https://github.com/apache/kafka/pull/13653#discussion_r1182836931
##########
server-common/src/main/java/org/apache/kafka/timeline/SnapshottableHashTable.java:
##########
@@ -113,7 +113,7 @@ public void mergeFrom(long epoch, Delta source) {
HashTier<T> other = (HashTier<T>) source;
// As an optimization, the deltaTable might not exist for a new key
// as there is no previous value
- if (other.deltaTable != null) {
+ if (deltaTable != null && other.deltaTable != null) {
Review Comment:
Hmm, if our deltaTable doesn't exist here, we should create it like this, right?
```
if (other.deltaTable != null) {
List<T> list = new ArrayList<>();
Object[] otherElements = other.deltaTable.baseElements();
for (int slot = 0; slot < otherElements.length; slot++) {
BaseHashTable.unpackSlot(list, otherElements, slot);
for (T element : list) {
// When merging in a later hash tier, we want to keep only the elements
// that were present at our epoch.
if (element.startEpoch() <= epoch) {
if (deltaTable == null) {
deltaTable = new BaseHashTable<>(1);
}
deltaTable.baseAddOrReplace(element);
}
}
}
}
```
Otherwise we're not merging in all the stuff we should be from the later snapshot
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org