You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/06/30 09:58:45 UTC
[GitHub] [geode] jujoramos opened a new pull request #5329: [WIP] - GEODE-8029: Allow OplogEntryIdSet to Overflow
jujoramos opened a new pull request #5329:
URL: https://github.com/apache/geode/pull/5329
Do not delete drf files during member startup as that should be only
done by the compactor thread. Instead, allow the OplogEntryIdSet to
grow over the default capacity and log a warning message instructing
the user to manually compact the disk-stores.
- Added unit tests.
- Replaced usages of 'junit.Assert' by 'assertj'.
- Modified DiskStoreImpl.deadRecordCount to return long instead of int.
- Added internal overflow implementation to the OplogEntryIdSet so it can
grow above the default limit.
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [X] Is your initial contribution a single, squashed commit?
- [X] Does `gradlew build` run cleanly?
- [X] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jujoramos merged pull request #5329: GEODE-8029: Allow OplogEntryIdSet to Overflow
Posted by GitBox <gi...@apache.org>.
jujoramos merged pull request #5329:
URL: https://github.com/apache/geode/pull/5329
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #5329: GEODE-8029: Allow OplogEntryIdSet to Overflow
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #5329:
URL: https://github.com/apache/geode/pull/5329#discussion_r448560371
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java
##########
@@ -3517,33 +3518,86 @@ public String toString() {
}
/**
- * Set of OplogEntryIds (longs). Memory is optimized by using an int[] for ids in the unsigned int
- * range.
+ * Set of OplogEntryIds (longs).
+ * Memory is optimized by using an int[] for ids in the unsigned int range.
+ * By default we can't have more than 805306401 ids for a load factor of 0.75, the internal lists
+ * are used to overcome this limit, allowing the disk-store to recover successfully (the internal
+ * class is **only** used during recovery to read all deleted entries).
*/
static class OplogEntryIdSet {
- private final IntOpenHashSet ints = new IntOpenHashSet((int) INVALID_ID);
- private final LongOpenHashSet longs = new LongOpenHashSet((int) INVALID_ID);
+ private final List<IntOpenHashSet> allInts;
+ private final List<LongOpenHashSet> allLongs;
+ private final AtomicReference<IntOpenHashSet> currentInts;
+ private final AtomicReference<LongOpenHashSet> currentLongs;
+
+ // For testing purposes only.
+ @VisibleForTesting
+ OplogEntryIdSet(List<IntOpenHashSet> allInts, List<LongOpenHashSet> allLongs) {
+ this.allInts = allInts;
+ this.currentInts = new AtomicReference<>(this.allInts.get(0));
+
+ this.allLongs = allLongs;
+ this.currentLongs = new AtomicReference<>(this.allLongs.get(0));
+ }
+
+ public OplogEntryIdSet() {
+ IntOpenHashSet intHashSet = new IntOpenHashSet((int) INVALID_ID);
+ this.allInts = new ArrayList<>();
+ this.allInts.add(intHashSet);
+ this.currentInts = new AtomicReference<>(intHashSet);
+
+ LongOpenHashSet longHashSet = new LongOpenHashSet((int) INVALID_ID);
+ this.allLongs = new ArrayList<>();
+ this.allLongs.add(longHashSet);
+ this.currentLongs = new AtomicReference<>(longHashSet);
+ }
public void add(long id) {
if (id == 0) {
throw new IllegalArgumentException();
- } else if (id > 0 && id <= 0x00000000FFFFFFFFL) {
- this.ints.add((int) id);
- } else {
- this.longs.add(id);
+ }
+
+ try {
+ if (id > 0 && id <= 0x00000000FFFFFFFFL) {
+ this.currentInts.get().add((int) id);
+ } else {
+ this.currentLongs.get().add(id);
+ }
+ } catch (IllegalArgumentException illegalArgumentException) {
+ // See GEODE-8029.
+ // Too many entries on the accumulated drf files, overflow and continue.
+ logger.warn(
+ "There are too many entries within the disk-store, please execute an offline compaction.",
Review comment:
"Too many entries", this is referring to deleted entries, right? In that case how about changing it
to "large number of deleted entries"?
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java
##########
@@ -3517,33 +3518,86 @@ public String toString() {
}
/**
- * Set of OplogEntryIds (longs). Memory is optimized by using an int[] for ids in the unsigned int
- * range.
+ * Set of OplogEntryIds (longs).
+ * Memory is optimized by using an int[] for ids in the unsigned int range.
+ * By default we can't have more than 805306401 ids for a load factor of 0.75, the internal lists
+ * are used to overcome this limit, allowing the disk-store to recover successfully (the internal
+ * class is **only** used during recovery to read all deleted entries).
*/
static class OplogEntryIdSet {
- private final IntOpenHashSet ints = new IntOpenHashSet((int) INVALID_ID);
- private final LongOpenHashSet longs = new LongOpenHashSet((int) INVALID_ID);
+ private final List<IntOpenHashSet> allInts;
+ private final List<LongOpenHashSet> allLongs;
+ private final AtomicReference<IntOpenHashSet> currentInts;
+ private final AtomicReference<LongOpenHashSet> currentLongs;
+
+ // For testing purposes only.
+ @VisibleForTesting
+ OplogEntryIdSet(List<IntOpenHashSet> allInts, List<LongOpenHashSet> allLongs) {
+ this.allInts = allInts;
+ this.currentInts = new AtomicReference<>(this.allInts.get(0));
+
+ this.allLongs = allLongs;
+ this.currentLongs = new AtomicReference<>(this.allLongs.get(0));
+ }
+
+ public OplogEntryIdSet() {
+ IntOpenHashSet intHashSet = new IntOpenHashSet((int) INVALID_ID);
+ this.allInts = new ArrayList<>();
+ this.allInts.add(intHashSet);
+ this.currentInts = new AtomicReference<>(intHashSet);
+
+ LongOpenHashSet longHashSet = new LongOpenHashSet((int) INVALID_ID);
+ this.allLongs = new ArrayList<>();
+ this.allLongs.add(longHashSet);
+ this.currentLongs = new AtomicReference<>(longHashSet);
+ }
public void add(long id) {
if (id == 0) {
throw new IllegalArgumentException();
- } else if (id > 0 && id <= 0x00000000FFFFFFFFL) {
- this.ints.add((int) id);
- } else {
- this.longs.add(id);
+ }
+
+ try {
+ if (id > 0 && id <= 0x00000000FFFFFFFFL) {
+ this.currentInts.get().add((int) id);
+ } else {
+ this.currentLongs.get().add(id);
+ }
+ } catch (IllegalArgumentException illegalArgumentException) {
+ // See GEODE-8029.
+ // Too many entries on the accumulated drf files, overflow and continue.
+ logger.warn(
+ "There are too many entries within the disk-store, please execute an offline compaction.",
+ illegalArgumentException);
+
+ // Overflow to the next [Int|Long]OpenHashSet and continue.
+ if (id > 0 && id <= 0x00000000FFFFFFFFL) {
Review comment:
@dschneider-pivotal
Is there a scenario where the same ID is added many times? In that case, earlier we had only one id set, and there was no chance of duplicate. Now with the new approach, the same ID could be present in multiple id sets.
One of the caller of add is "offlineCompact()" its iterating over live entries and calling add...Will there be two live entries with the same id?
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java
##########
@@ -3517,33 +3518,86 @@ public String toString() {
}
/**
- * Set of OplogEntryIds (longs). Memory is optimized by using an int[] for ids in the unsigned int
- * range.
+ * Set of OplogEntryIds (longs).
+ * Memory is optimized by using an int[] for ids in the unsigned int range.
+ * By default we can't have more than 805306401 ids for a load factor of 0.75, the internal lists
+ * are used to overcome this limit, allowing the disk-store to recover successfully (the internal
+ * class is **only** used during recovery to read all deleted entries).
*/
static class OplogEntryIdSet {
- private final IntOpenHashSet ints = new IntOpenHashSet((int) INVALID_ID);
- private final LongOpenHashSet longs = new LongOpenHashSet((int) INVALID_ID);
+ private final List<IntOpenHashSet> allInts;
+ private final List<LongOpenHashSet> allLongs;
+ private final AtomicReference<IntOpenHashSet> currentInts;
+ private final AtomicReference<LongOpenHashSet> currentLongs;
+
+ // For testing purposes only.
+ @VisibleForTesting
+ OplogEntryIdSet(List<IntOpenHashSet> allInts, List<LongOpenHashSet> allLongs) {
+ this.allInts = allInts;
+ this.currentInts = new AtomicReference<>(this.allInts.get(0));
+
+ this.allLongs = allLongs;
+ this.currentLongs = new AtomicReference<>(this.allLongs.get(0));
+ }
+
+ public OplogEntryIdSet() {
+ IntOpenHashSet intHashSet = new IntOpenHashSet((int) INVALID_ID);
+ this.allInts = new ArrayList<>();
+ this.allInts.add(intHashSet);
+ this.currentInts = new AtomicReference<>(intHashSet);
+
+ LongOpenHashSet longHashSet = new LongOpenHashSet((int) INVALID_ID);
+ this.allLongs = new ArrayList<>();
+ this.allLongs.add(longHashSet);
+ this.currentLongs = new AtomicReference<>(longHashSet);
+ }
public void add(long id) {
if (id == 0) {
throw new IllegalArgumentException();
- } else if (id > 0 && id <= 0x00000000FFFFFFFFL) {
- this.ints.add((int) id);
- } else {
- this.longs.add(id);
+ }
+
+ try {
+ if (id > 0 && id <= 0x00000000FFFFFFFFL) {
+ this.currentInts.get().add((int) id);
+ } else {
+ this.currentLongs.get().add(id);
+ }
+ } catch (IllegalArgumentException illegalArgumentException) {
+ // See GEODE-8029.
+ // Too many entries on the accumulated drf files, overflow and continue.
+ logger.warn(
+ "There are too many entries within the disk-store, please execute an offline compaction.",
Review comment:
Do we need to pass "illegalArgumentExcpetion" to warning message? It may not be useful for end user.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jujoramos commented on a change in pull request #5329: GEODE-8029: Allow OplogEntryIdSet to Overflow
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #5329:
URL: https://github.com/apache/geode/pull/5329#discussion_r448898069
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
##########
@@ -937,17 +937,11 @@ void initAfterRecovery(boolean offline) {
// this.crf.raf.seek(this.crf.currSize);
} else if (!offline) {
// drf exists but crf has been deleted (because it was empty).
+ // I don't think the drf needs to be opened. It is only used during
+ // recovery.
+ // At some point the compacter my identify that it can be deleted.
Review comment:
Thanks @dschneider-pivotal!. I'll fix the type and create another ticket for the long term solution.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jujoramos commented on pull request #5329: GEODE-8029: Allow OplogEntryIdSet to Overflow
Posted by GitBox <gi...@apache.org>.
jujoramos commented on pull request #5329:
URL: https://github.com/apache/geode/pull/5329#issuecomment-652921469
@pivotal-eshu
> If we no longer need the OplogEntryIdSet after the recovery, shall we clear these sets once it is done?
There are no references to the `OplogEntryIdSet` instance once we finish the recovery, so the class itself (along with all internal structures) will eventually be removed by the garbage collector.
> With these many entryIds in drf files, it will also take longer time to recover. I think Darrel's idea for removing unnecessary drf files will help in this case as well.
Agreed, I'll create another ticket for that.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5329: GEODE-8029: Allow OplogEntryIdSet to Overflow
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5329:
URL: https://github.com/apache/geode/pull/5329#discussion_r448518869
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/Oplog.java
##########
@@ -937,17 +937,11 @@ void initAfterRecovery(boolean offline) {
// this.crf.raf.seek(this.crf.currSize);
} else if (!offline) {
// drf exists but crf has been deleted (because it was empty).
+ // I don't think the drf needs to be opened. It is only used during
+ // recovery.
+ // At some point the compacter my identify that it can be deleted.
Review comment:
change "my" to "may"
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5329: GEODE-8029: Allow OplogEntryIdSet to Overflow
Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5329:
URL: https://github.com/apache/geode/pull/5329#discussion_r448615239
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java
##########
@@ -3517,33 +3518,86 @@ public String toString() {
}
/**
- * Set of OplogEntryIds (longs). Memory is optimized by using an int[] for ids in the unsigned int
- * range.
+ * Set of OplogEntryIds (longs).
+ * Memory is optimized by using an int[] for ids in the unsigned int range.
+ * By default we can't have more than 805306401 ids for a load factor of 0.75, the internal lists
+ * are used to overcome this limit, allowing the disk-store to recover successfully (the internal
+ * class is **only** used during recovery to read all deleted entries).
*/
static class OplogEntryIdSet {
- private final IntOpenHashSet ints = new IntOpenHashSet((int) INVALID_ID);
- private final LongOpenHashSet longs = new LongOpenHashSet((int) INVALID_ID);
+ private final List<IntOpenHashSet> allInts;
+ private final List<LongOpenHashSet> allLongs;
+ private final AtomicReference<IntOpenHashSet> currentInts;
+ private final AtomicReference<LongOpenHashSet> currentLongs;
+
+ // For testing purposes only.
+ @VisibleForTesting
+ OplogEntryIdSet(List<IntOpenHashSet> allInts, List<LongOpenHashSet> allLongs) {
+ this.allInts = allInts;
+ this.currentInts = new AtomicReference<>(this.allInts.get(0));
+
+ this.allLongs = allLongs;
+ this.currentLongs = new AtomicReference<>(this.allLongs.get(0));
+ }
+
+ public OplogEntryIdSet() {
+ IntOpenHashSet intHashSet = new IntOpenHashSet((int) INVALID_ID);
+ this.allInts = new ArrayList<>();
+ this.allInts.add(intHashSet);
+ this.currentInts = new AtomicReference<>(intHashSet);
+
+ LongOpenHashSet longHashSet = new LongOpenHashSet((int) INVALID_ID);
+ this.allLongs = new ArrayList<>();
+ this.allLongs.add(longHashSet);
+ this.currentLongs = new AtomicReference<>(longHashSet);
+ }
public void add(long id) {
if (id == 0) {
throw new IllegalArgumentException();
- } else if (id > 0 && id <= 0x00000000FFFFFFFFL) {
- this.ints.add((int) id);
- } else {
- this.longs.add(id);
+ }
+
+ try {
+ if (id > 0 && id <= 0x00000000FFFFFFFFL) {
+ this.currentInts.get().add((int) id);
+ } else {
+ this.currentLongs.get().add(id);
+ }
+ } catch (IllegalArgumentException illegalArgumentException) {
+ // See GEODE-8029.
+ // Too many entries on the accumulated drf files, overflow and continue.
+ logger.warn(
+ "There are too many entries within the disk-store, please execute an offline compaction.",
+ illegalArgumentException);
+
+ // Overflow to the next [Int|Long]OpenHashSet and continue.
+ if (id > 0 && id <= 0x00000000FFFFFFFFL) {
Review comment:
I don't think so. The ids are unique and should only be deleted once from a drf. If this did happen it would just cause the size to be larger than it should be. So you could review the code that uses the size of this class.
You could prevent this from having each add first call contains which shouldn't slow down the add too much.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jujoramos commented on a change in pull request #5329: GEODE-8029: Allow OplogEntryIdSet to Overflow
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #5329:
URL: https://github.com/apache/geode/pull/5329#discussion_r448898320
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java
##########
@@ -3517,33 +3518,86 @@ public String toString() {
}
/**
- * Set of OplogEntryIds (longs). Memory is optimized by using an int[] for ids in the unsigned int
- * range.
+ * Set of OplogEntryIds (longs).
+ * Memory is optimized by using an int[] for ids in the unsigned int range.
+ * By default we can't have more than 805306401 ids for a load factor of 0.75, the internal lists
+ * are used to overcome this limit, allowing the disk-store to recover successfully (the internal
+ * class is **only** used during recovery to read all deleted entries).
*/
static class OplogEntryIdSet {
- private final IntOpenHashSet ints = new IntOpenHashSet((int) INVALID_ID);
- private final LongOpenHashSet longs = new LongOpenHashSet((int) INVALID_ID);
+ private final List<IntOpenHashSet> allInts;
+ private final List<LongOpenHashSet> allLongs;
+ private final AtomicReference<IntOpenHashSet> currentInts;
+ private final AtomicReference<LongOpenHashSet> currentLongs;
+
+ // For testing purposes only.
+ @VisibleForTesting
+ OplogEntryIdSet(List<IntOpenHashSet> allInts, List<LongOpenHashSet> allLongs) {
+ this.allInts = allInts;
+ this.currentInts = new AtomicReference<>(this.allInts.get(0));
+
+ this.allLongs = allLongs;
+ this.currentLongs = new AtomicReference<>(this.allLongs.get(0));
+ }
+
+ public OplogEntryIdSet() {
+ IntOpenHashSet intHashSet = new IntOpenHashSet((int) INVALID_ID);
+ this.allInts = new ArrayList<>();
+ this.allInts.add(intHashSet);
+ this.currentInts = new AtomicReference<>(intHashSet);
+
+ LongOpenHashSet longHashSet = new LongOpenHashSet((int) INVALID_ID);
+ this.allLongs = new ArrayList<>();
+ this.allLongs.add(longHashSet);
+ this.currentLongs = new AtomicReference<>(longHashSet);
+ }
public void add(long id) {
if (id == 0) {
throw new IllegalArgumentException();
- } else if (id > 0 && id <= 0x00000000FFFFFFFFL) {
- this.ints.add((int) id);
- } else {
- this.longs.add(id);
+ }
+
+ try {
+ if (id > 0 && id <= 0x00000000FFFFFFFFL) {
+ this.currentInts.get().add((int) id);
+ } else {
+ this.currentLongs.get().add(id);
+ }
+ } catch (IllegalArgumentException illegalArgumentException) {
+ // See GEODE-8029.
+ // Too many entries on the accumulated drf files, overflow and continue.
+ logger.warn(
+ "There are too many entries within the disk-store, please execute an offline compaction.",
Review comment:
👍
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jujoramos commented on a change in pull request #5329: GEODE-8029: Allow OplogEntryIdSet to Overflow
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #5329:
URL: https://github.com/apache/geode/pull/5329#discussion_r448907611
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java
##########
@@ -3517,33 +3518,86 @@ public String toString() {
}
/**
- * Set of OplogEntryIds (longs). Memory is optimized by using an int[] for ids in the unsigned int
- * range.
+ * Set of OplogEntryIds (longs).
+ * Memory is optimized by using an int[] for ids in the unsigned int range.
+ * By default we can't have more than 805306401 ids for a load factor of 0.75, the internal lists
+ * are used to overcome this limit, allowing the disk-store to recover successfully (the internal
+ * class is **only** used during recovery to read all deleted entries).
*/
static class OplogEntryIdSet {
- private final IntOpenHashSet ints = new IntOpenHashSet((int) INVALID_ID);
- private final LongOpenHashSet longs = new LongOpenHashSet((int) INVALID_ID);
+ private final List<IntOpenHashSet> allInts;
+ private final List<LongOpenHashSet> allLongs;
+ private final AtomicReference<IntOpenHashSet> currentInts;
+ private final AtomicReference<LongOpenHashSet> currentLongs;
+
+ // For testing purposes only.
+ @VisibleForTesting
+ OplogEntryIdSet(List<IntOpenHashSet> allInts, List<LongOpenHashSet> allLongs) {
+ this.allInts = allInts;
+ this.currentInts = new AtomicReference<>(this.allInts.get(0));
+
+ this.allLongs = allLongs;
+ this.currentLongs = new AtomicReference<>(this.allLongs.get(0));
+ }
+
+ public OplogEntryIdSet() {
+ IntOpenHashSet intHashSet = new IntOpenHashSet((int) INVALID_ID);
+ this.allInts = new ArrayList<>();
+ this.allInts.add(intHashSet);
+ this.currentInts = new AtomicReference<>(intHashSet);
+
+ LongOpenHashSet longHashSet = new LongOpenHashSet((int) INVALID_ID);
+ this.allLongs = new ArrayList<>();
+ this.allLongs.add(longHashSet);
+ this.currentLongs = new AtomicReference<>(longHashSet);
+ }
public void add(long id) {
if (id == 0) {
throw new IllegalArgumentException();
- } else if (id > 0 && id <= 0x00000000FFFFFFFFL) {
- this.ints.add((int) id);
- } else {
- this.longs.add(id);
+ }
+
+ try {
+ if (id > 0 && id <= 0x00000000FFFFFFFFL) {
+ this.currentInts.get().add((int) id);
+ } else {
+ this.currentLongs.get().add(id);
+ }
+ } catch (IllegalArgumentException illegalArgumentException) {
+ // See GEODE-8029.
+ // Too many entries on the accumulated drf files, overflow and continue.
+ logger.warn(
+ "There are too many entries within the disk-store, please execute an offline compaction.",
+ illegalArgumentException);
+
+ // Overflow to the next [Int|Long]OpenHashSet and continue.
+ if (id > 0 && id <= 0x00000000FFFFFFFFL) {
Review comment:
@agingade: I think we're good on this regard, `ids` are unique and the actual size of the `OplogEntryIdSet` is _**only used**_ by `offline-validate` / `offline-compact` commands. That is, if we ever report a bigger number for `deadRecordCount` (set through `OplogEntryIdSet.size()`) because there are duplicated `ids` (shouldn't happen), the user would know that the `disk-store` should be compacted anyways.
The real problem would happen if we ever report there are not compact-able entries when there actually are, which is not the case here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] agingade commented on a change in pull request #5329: GEODE-8029: Allow OplogEntryIdSet to Overflow
Posted by GitBox <gi...@apache.org>.
agingade commented on a change in pull request #5329:
URL: https://github.com/apache/geode/pull/5329#discussion_r449178722
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/DiskStoreImpl.java
##########
@@ -3517,33 +3518,86 @@ public String toString() {
}
/**
- * Set of OplogEntryIds (longs). Memory is optimized by using an int[] for ids in the unsigned int
- * range.
+ * Set of OplogEntryIds (longs).
+ * Memory is optimized by using an int[] for ids in the unsigned int range.
+ * By default we can't have more than 805306401 ids for a load factor of 0.75, the internal lists
+ * are used to overcome this limit, allowing the disk-store to recover successfully (the internal
+ * class is **only** used during recovery to read all deleted entries).
*/
static class OplogEntryIdSet {
- private final IntOpenHashSet ints = new IntOpenHashSet((int) INVALID_ID);
- private final LongOpenHashSet longs = new LongOpenHashSet((int) INVALID_ID);
+ private final List<IntOpenHashSet> allInts;
+ private final List<LongOpenHashSet> allLongs;
+ private final AtomicReference<IntOpenHashSet> currentInts;
+ private final AtomicReference<LongOpenHashSet> currentLongs;
+
+ // For testing purposes only.
+ @VisibleForTesting
+ OplogEntryIdSet(List<IntOpenHashSet> allInts, List<LongOpenHashSet> allLongs) {
+ this.allInts = allInts;
+ this.currentInts = new AtomicReference<>(this.allInts.get(0));
+
+ this.allLongs = allLongs;
+ this.currentLongs = new AtomicReference<>(this.allLongs.get(0));
+ }
+
+ public OplogEntryIdSet() {
+ IntOpenHashSet intHashSet = new IntOpenHashSet((int) INVALID_ID);
+ this.allInts = new ArrayList<>();
+ this.allInts.add(intHashSet);
+ this.currentInts = new AtomicReference<>(intHashSet);
+
+ LongOpenHashSet longHashSet = new LongOpenHashSet((int) INVALID_ID);
+ this.allLongs = new ArrayList<>();
+ this.allLongs.add(longHashSet);
+ this.currentLongs = new AtomicReference<>(longHashSet);
+ }
public void add(long id) {
if (id == 0) {
throw new IllegalArgumentException();
- } else if (id > 0 && id <= 0x00000000FFFFFFFFL) {
- this.ints.add((int) id);
- } else {
- this.longs.add(id);
+ }
+
+ try {
+ if (id > 0 && id <= 0x00000000FFFFFFFFL) {
+ this.currentInts.get().add((int) id);
+ } else {
+ this.currentLongs.get().add(id);
+ }
+ } catch (IllegalArgumentException illegalArgumentException) {
+ // See GEODE-8029.
+ // Too many entries on the accumulated drf files, overflow and continue.
+ logger.warn(
+ "There are too many entries within the disk-store, please execute an offline compaction.",
+ illegalArgumentException);
+
+ // Overflow to the next [Int|Long]OpenHashSet and continue.
+ if (id > 0 && id <= 0x00000000FFFFFFFFL) {
Review comment:
Thanks for the clarification.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org