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

KAFKA-4379 Followup: Avoid sending to changelog while restoring InMemoryLRUCache #2908

Closed
wants to merge 6 commits into from

Conversation

guozhangwang
Copy link
Contributor

@guozhangwang guozhangwang commented Apr 25, 2017

  1. Added a flag to indicate if it is restoring or not in the LRU Store; since we only have a restore callback we have to set it each time applying the change.
  2. Fixed the corresponding unit test, plus some minor cleaning up.

@guozhangwang
Copy link
Contributor Author

ping @norwood @dguy for reviews.

The reason that I think it is safe to remove the listener is that with either log truncation and log compaction, we can actually prove that after replaying the lru store's image will not contain any unexpected records, i.e.: any record that will show up in the restored lru will definitely be in the original lru, even with delete operations supported in the lru store. I'd omit the proof here but we can chat offline if you are interested.

With that, I think we then do not need to log the evicted records anymore from the changelog topic, and it is safe to just remove this piece of logic here.

Copy link
Contributor

@dguy dguy left a comment

Choose a reason for hiding this comment

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

@guozhangwang what is the downside of this? Is it that in the case it is backed by a compacted topic that items won't ever be removed as tombstones aren't sent?

@norwood
Copy link
Contributor

norwood commented Apr 25, 2017

time for a new compaction policy based on message count

@asfbot
Copy link

asfbot commented Apr 25, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3166/
Test FAILed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented Apr 25, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3167/
Test FAILed (JDK 7 and Scala 2.10).

@guozhangwang
Copy link
Contributor Author

@norwood @dguy @mjsax I thought about that too. What I concluded was that if we record the evicted record along with log compaction, we actually will not get correct restored state compared with the original lru store ("state" include the ordering of the filled records which will affect future eviction decisions after restoration).

Also I did some simple test to verify that with recorded eviction record, restored state may actually be incorrect, so admittedly the downside of ever growing changelog is worse, but that is necessary for correctness.

On the other hand, since as we have discussed, we can write the offset in checkpoint file as well for lru because of the proven property mentioned above to reduce restoration latency (this is a niche optimization for lru only so not to much work to add).

@guozhangwang
Copy link
Contributor Author

Chatted with @norwood offline, and we decided to go with the alternative direction to still log evicted records only after restoration.

@asfbot
Copy link

asfbot commented Apr 25, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/3172/
Test FAILed (JDK 8 and Scala 2.11).

@guozhangwang
Copy link
Contributor Author

@norwood @dguy @mjsax open for review again.

if (listener != null) listener.apply(key, eldest.getValue());
return true;
boolean evict = super.size() > maxCacheSize;
if (evict && !restoring && listener != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do the updates on restoring from the StateRestoreCallback always happen in the same thread?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

👏

Copy link
Member

@mjsax mjsax left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@dguy dguy left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@hachikuji hachikuji left a comment

Choose a reason for hiding this comment

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

LGTM. I'll merge after @guozhangwang changes the title.

@guozhangwang guozhangwang changed the title KAFKA-4379 Followup: Remove eviction listener from InMemoryLRUCache KAFKA-4379 Followup: Avoid sending to changelog while restoring InMemoryLRUCache Apr 27, 2017
@asfbot
Copy link

asfbot commented Apr 27, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/3242/
Test FAILed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented Apr 27, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3233/
Test FAILed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented Apr 27, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3238/
Test FAILed (JDK 7 and Scala 2.10).

@guozhangwang
Copy link
Contributor Author

local unit test passed.

@asfbot
Copy link

asfbot commented Apr 27, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/3236/
Test FAILed (JDK 8 and Scala 2.12).

@asfbot
Copy link

asfbot commented Apr 27, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/3245/
Test PASSed (JDK 8 and Scala 2.11).

@asfbot
Copy link

asfbot commented Apr 28, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/3240/
Test PASSed (JDK 7 and Scala 2.10).

@asfgit asfgit closed this in ee8c5e2 Apr 28, 2017
@guozhangwang guozhangwang deleted the K4379-remove-listener branch July 15, 2017 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants