You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "umesh9794 (via GitHub)" <gi...@apache.org> on 2026/06/19 10:49:09 UTC
[PR] HDDS-15582. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
umesh9794 opened a new pull request, #10549:
URL: https://github.com/apache/ozone/pull/10549
## What changes were proposed in this pull request?
Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15601
## How was this patch tested?
Added unit test to cover the code changes
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "umesh9794 (via GitHub)" <gi...@apache.org>.
umesh9794 commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-4808258131
Sure, here is the benchmark figures based on test:
| Approach | Perf Test Execution Time | Notes |
|--------|--------|--------|
| Before (master baseline) | 82.1 s | Delete-all + re-insert (replaceUnhealthyContainerRecordsAtomically) |
| After this PR | 18.4 s | Insert / update / delete-stale only (syncUnhealthyContainerRecordsAtomically) |
Improvement vs original: ~4.5× faster (82s → 18s)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "umesh9794 (via GitHub)" <gi...@apache.org>.
umesh9794 commented on code in PR #10549:
URL: https://github.com/apache/ozone/pull/10549#discussion_r3542750740
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -227,6 +291,43 @@ private int deleteScmStatesForContainers(DSLContext dslContext,
return totalDeleted;
}
+ private void deleteStaleUnhealthyRecords(DSLContext dslContext,
+ List<ContainerStateKey> staleKeys) {
+ if (staleKeys.isEmpty()) {
+ return;
+ }
+ for (ContainerStateKey key : staleKeys) {
+ dslContext.deleteFrom(UNHEALTHY_CONTAINERS)
+ .where(UNHEALTHY_CONTAINERS.CONTAINER_ID.eq(key.getContainerId()))
+ .and(UNHEALTHY_CONTAINERS.CONTAINER_STATE.eq(key.getContainerState()))
+ .execute();
+ }
+ }
+
+ private void batchUpdateInChunks(DSLContext dslContext,
+ List<UnhealthyContainerRecord> recs) {
+ if (recs.isEmpty()) {
+ return;
+ }
+ for (int from = 0; from < recs.size(); from += BATCH_INSERT_CHUNK_SIZE) {
Review Comment:
Good catch. Yes, batch update are missing, it only does batching for inserts as of now. Let me cover the batching in updated and deletes too.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-4891724024
> Sure, here is the benchmark figures based on test:
> Approach Perf Test Execution Time Notes
> Before (master baseline) 82.1 s Delete-all + re-insert (replaceUnhealthyContainerRecordsAtomically)
> After this PR 18.4 s Insert / update / delete-stale only (syncUnhealthyContainerRecordsAtomically)
>
> Improvement vs original: ~4.5× faster (82s → 18s)
Are the above performance figures from previous insert and delete test where you added "`@Slow("HDDS-15582")`" ? Also I don't see you removed this tag if performance improved ?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "umesh9794 (via GitHub)" <gi...@apache.org>.
umesh9794 commented on code in PR #10549:
URL: https://github.com/apache/ozone/pull/10549#discussion_r3542755105
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -179,6 +179,70 @@ public void batchDeleteSCMStatesForContainers(List<Long> containerIds) {
totalDeleted, containerIds.size());
}
+ /**
+ * Atomically syncs unhealthy rows for a scan chunk: insert new
+ * (container_id, state) pairs, update existing pairs, and delete only stale
+ * pairs that are no longer unhealthy.
+ *
+ * <p>Unlike {@link #replaceUnhealthyContainerRecordsAtomically}, this does not
+ * delete all SCM states for every container that previously had a row. It
+ * removes only keys present in {@code existingByKey} but absent from the
+ * desired scan result (containers that recovered or changed state).</p>
+ *
+ * @param existingByKey prior rows for this chunk, keyed by
+ * (container_id, container_state)
+ * @param desiredRecords unhealthy rows produced by the current scan
+ */
+ public void syncUnhealthyContainerRecordsAtomically(
+ Map<ContainerStateKey, Long> existingByKey,
+ List<UnhealthyContainerRecord> desiredRecords) {
+ if ((existingByKey == null || existingByKey.isEmpty())
+ && (desiredRecords == null || desiredRecords.isEmpty())) {
+ return;
+ }
+
+ Map<ContainerStateKey, UnhealthyContainerRecord> desiredByKey = new HashMap<>();
+ if (desiredRecords != null) {
+ for (UnhealthyContainerRecord record : desiredRecords) {
+ desiredByKey.put(new ContainerStateKey(record.getContainerId(),
+ record.getContainerState()), record);
+ }
+ }
+
+ Map<ContainerStateKey, Long> existing =
+ existingByKey == null ? new HashMap<>() : existingByKey;
+
+ List<ContainerStateKey> staleKeys = new ArrayList<>();
+ List<UnhealthyContainerRecord> toInsert = new ArrayList<>();
+ List<UnhealthyContainerRecord> toUpdate = new ArrayList<>();
+
+ for (ContainerStateKey key : existing.keySet()) {
+ if (!desiredByKey.containsKey(key)) {
+ staleKeys.add(key);
+ }
+ }
+ for (UnhealthyContainerRecord record : desiredByKey.values()) {
+ ContainerStateKey key = new ContainerStateKey(record.getContainerId(),
+ record.getContainerState());
+ if (existing.containsKey(key)) {
+ toUpdate.add(record);
+ } else {
+ toInsert.add(record);
+ }
+ }
+
+ DSLContext dslContext = containerSchemaDefinition.getDSLContext();
+ dslContext.transaction(configuration -> {
+ DSLContext txContext = configuration.dsl();
+ deleteStaleUnhealthyRecords(txContext, staleKeys);
+ batchInsertInChunks(txContext, toInsert);
+ batchUpdateInChunks(txContext, toUpdate);
Review Comment:
Sure, will do 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #10549:
URL: https://github.com/apache/ozone/pull/10549#discussion_r3450051723
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -205,28 +223,76 @@ public void replaceUnhealthyContainerRecordsAtomically(
private int deleteScmStatesForContainers(DSLContext dslContext,
List<Long> containerIds) {
+
+ // Sort and de-duplicate so contiguous runs are detectable regardless of the
+ // order in which the caller supplied the ids.
+ List<Long> sortedIds = containerIds.stream()
+ .distinct()
+ .sorted()
+ .collect(Collectors.toList());
+
int totalDeleted = 0;
+ List<Long> inClauseBatch = new ArrayList<>(MAX_IN_CLAUSE_CHUNK_SIZE);
+
+ int i = 0;
+ while (i < sortedIds.size()) {
+ int runStart = i;
+ while (i + 1 < sortedIds.size()
+ && sortedIds.get(i + 1) == sortedIds.get(i) + 1) {
+ i++;
+ }
- for (int from = 0; from < containerIds.size(); from += MAX_IN_CLAUSE_CHUNK_SIZE) {
- int to = Math.min(from + MAX_IN_CLAUSE_CHUNK_SIZE, containerIds.size());
- List<Long> chunk = containerIds.subList(from, to);
-
- int deleted = dslContext.deleteFrom(UNHEALTHY_CONTAINERS)
- .where(UNHEALTHY_CONTAINERS.CONTAINER_ID.in(chunk))
- .and(UNHEALTHY_CONTAINERS.CONTAINER_STATE.in(
- UnHealthyContainerStates.MISSING.toString(),
- UnHealthyContainerStates.EMPTY_MISSING.toString(),
- UnHealthyContainerStates.UNDER_REPLICATED.toString(),
- UnHealthyContainerStates.OVER_REPLICATED.toString(),
- UnHealthyContainerStates.MIS_REPLICATED.toString(),
- UnHealthyContainerStates.NEGATIVE_SIZE.toString(),
- UnHealthyContainerStates.REPLICA_MISMATCH.toString()))
- .execute();
- totalDeleted += deleted;
+ if (i - runStart + 1 >= MIN_RANGE_DELETE_RUN) {
+ // Long contiguous run: a single constant-size BETWEEN removes the whole
+ // range without straining Derby's per-statement bytecode budget.
+ totalDeleted += deleteScmStatesByRange(dslContext,
+ sortedIds.get(runStart), sortedIds.get(i));
Review Comment:
Using `BETWEEN` instead of `IN` for contiguous range of IDs is a good idea.
But I don't think the current change has much effect on production behavior, a range of 1000+ "unhealthy" containers in Recon is not real life scenario. So it adds complexity without benefit.
We should apply this logic for shorter ranges, without executing too many small statements. For that, we need to find the threshold (number of container IDs) over which `BETWEEN n AND n+k OR` yields shorter code than the corresponding `IN` clause. Any number of IDs beyond that is a win.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "umesh9794 (via GitHub)" <gi...@apache.org>.
umesh9794 commented on code in PR #10549:
URL: https://github.com/apache/ozone/pull/10549#discussion_r3451509361
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -205,28 +223,76 @@ public void replaceUnhealthyContainerRecordsAtomically(
private int deleteScmStatesForContainers(DSLContext dslContext,
List<Long> containerIds) {
+
+ // Sort and de-duplicate so contiguous runs are detectable regardless of the
+ // order in which the caller supplied the ids.
+ List<Long> sortedIds = containerIds.stream()
+ .distinct()
+ .sorted()
+ .collect(Collectors.toList());
+
int totalDeleted = 0;
+ List<Long> inClauseBatch = new ArrayList<>(MAX_IN_CLAUSE_CHUNK_SIZE);
+
+ int i = 0;
+ while (i < sortedIds.size()) {
+ int runStart = i;
+ while (i + 1 < sortedIds.size()
+ && sortedIds.get(i + 1) == sortedIds.get(i) + 1) {
+ i++;
+ }
- for (int from = 0; from < containerIds.size(); from += MAX_IN_CLAUSE_CHUNK_SIZE) {
- int to = Math.min(from + MAX_IN_CLAUSE_CHUNK_SIZE, containerIds.size());
- List<Long> chunk = containerIds.subList(from, to);
-
- int deleted = dslContext.deleteFrom(UNHEALTHY_CONTAINERS)
- .where(UNHEALTHY_CONTAINERS.CONTAINER_ID.in(chunk))
- .and(UNHEALTHY_CONTAINERS.CONTAINER_STATE.in(
- UnHealthyContainerStates.MISSING.toString(),
- UnHealthyContainerStates.EMPTY_MISSING.toString(),
- UnHealthyContainerStates.UNDER_REPLICATED.toString(),
- UnHealthyContainerStates.OVER_REPLICATED.toString(),
- UnHealthyContainerStates.MIS_REPLICATED.toString(),
- UnHealthyContainerStates.NEGATIVE_SIZE.toString(),
- UnHealthyContainerStates.REPLICA_MISMATCH.toString()))
- .execute();
- totalDeleted += deleted;
+ if (i - runStart + 1 >= MIN_RANGE_DELETE_RUN) {
+ // Long contiguous run: a single constant-size BETWEEN removes the whole
+ // range without straining Derby's per-statement bytecode budget.
+ totalDeleted += deleteScmStatesByRange(dslContext,
+ sortedIds.get(runStart), sortedIds.get(i));
Review Comment:
> Using `BETWEEN` instead of `IN` for contiguous range of IDs is a good idea.
>
> But I don't think the current change has much effect on production behavior, a range of 1000+ "unhealthy" containers in Recon is not real life scenario. So it adds complexity without benefit.
>
> We should apply this logic for shorter ranges, without executing too many small statements. For that, we need to find the threshold (number of container IDs) over which `BETWEEN n AND n+k OR` yields shorter code than the corresponding `IN` clause. Any number of IDs beyond that is a win.
@adoroszlai I have applied the changes, 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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-5304848691
Thank you for your contribution. This PR is being closed due to inactivity. Please contact a maintainer if you would like to reopen it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #10549: HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager
URL: https://github.com/apache/ozone/pull/10549
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-5223399992
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "umesh9794 (via GitHub)" <gi...@apache.org>.
umesh9794 commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-4988744500
@devmadhuu I have added a file based derby test to compare master with this PR with file based derby, here are the outcomes :
**Test 7 (worst case: 1M rows rewritten, single transaction)**
In-memory vs File based Derby | master (delete-all + insert) | this PR (diff sync → all updates)
-- | -- | --
In-memory | ~82 s | ~18 s
File-based | ~87 s | >300 s → timed out
**Full file-based comparison**
Test | Operation | master | this PR
-- | -- | -- | --
1 | INSERT 1M | ~19 s | ~17.6 s
2 | COUNT(*) 1M | ~0.27 s | ~0.24 s
5 | Paginated read 200K | ~0.55 s | ~0.53 s
6 | Full 1M read | ~1.9 s | ~1.9 s
7 | Rewrite 1M, one tx | ~87 s | >300 s (timeout)
**In-memory:** no log flush, so 1M in-place UPDATEs (~18 s) beat master's bulk delete + insert (~82 s).
**File-based:** the PR's Test 7 issues 1M single-row UPDATEs inside one transaction — each is an index seek + in-place modify + WAL entry, and nothing commits until the end. On disk that's dramatically more expensive than master's bulk delete + append-style insert (which Derby handles efficiently). Hence >300 s vs 87 s.
But Test 7 is not reflects the production scenario IIUC. Production syncs ≤50k-container chunks (not 200k-in-one-transaction), and in steady state most rows are unchanged. The realistic measure is the 50k steady-state chunk:
this PR: ~3.9 s (skips ~45k, writes ~5k)
master would unconditionally delete + re-insert the entire working set every scan, regardless of churn.
So IMO the PR is a clear win in the real steady-state pattern, and a regression only in the artificial all-churn-in-one-giant-transaction case.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-4807257133
Please publish your performance figures and maths before and earlier
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "umesh9794 (via GitHub)" <gi...@apache.org>.
umesh9794 commented on code in PR #10549:
URL: https://github.com/apache/ozone/pull/10549#discussion_r3449968387
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -205,28 +223,76 @@ public void replaceUnhealthyContainerRecordsAtomically(
private int deleteScmStatesForContainers(DSLContext dslContext,
List<Long> containerIds) {
+
+ // Sort and de-duplicate so contiguous runs are detectable regardless of the
+ // order in which the caller supplied the ids.
+ List<Long> sortedIds = containerIds.stream()
+ .distinct()
+ .sorted()
+ .collect(Collectors.toList());
+
int totalDeleted = 0;
+ List<Long> inClauseBatch = new ArrayList<>(MAX_IN_CLAUSE_CHUNK_SIZE);
+
+ int i = 0;
+ while (i < sortedIds.size()) {
+ int runStart = i;
+ while (i + 1 < sortedIds.size()
+ && sortedIds.get(i + 1) == sortedIds.get(i) + 1) {
+ i++;
+ }
- for (int from = 0; from < containerIds.size(); from += MAX_IN_CLAUSE_CHUNK_SIZE) {
- int to = Math.min(from + MAX_IN_CLAUSE_CHUNK_SIZE, containerIds.size());
- List<Long> chunk = containerIds.subList(from, to);
-
- int deleted = dslContext.deleteFrom(UNHEALTHY_CONTAINERS)
- .where(UNHEALTHY_CONTAINERS.CONTAINER_ID.in(chunk))
- .and(UNHEALTHY_CONTAINERS.CONTAINER_STATE.in(
- UnHealthyContainerStates.MISSING.toString(),
- UnHealthyContainerStates.EMPTY_MISSING.toString(),
- UnHealthyContainerStates.UNDER_REPLICATED.toString(),
- UnHealthyContainerStates.OVER_REPLICATED.toString(),
- UnHealthyContainerStates.MIS_REPLICATED.toString(),
- UnHealthyContainerStates.NEGATIVE_SIZE.toString(),
- UnHealthyContainerStates.REPLICA_MISMATCH.toString()))
- .execute();
- totalDeleted += deleted;
+ if (i - runStart + 1 >= MIN_RANGE_DELETE_RUN) {
+ // Long contiguous run: a single constant-size BETWEEN removes the whole
+ // range without straining Derby's per-statement bytecode budget.
+ totalDeleted += deleteScmStatesByRange(dslContext,
+ sortedIds.get(runStart), sortedIds.get(i));
Review Comment:
I was trying to keep the `DELETE` statements with `BETWEEN` and `IN` clause separate to issue 2 `DELETE` statements. So in the worst case it falls back to the same IN chunking the code used before this change, so it's never worse than the old baseline of ceil(n / 1000). In best case it will issue just 1 statement for a fully contiguous block.
As the comments says above combining both might overflow Derby's per-statement bytecode limit (`Bytecode per method`).
If you prefer, I can combine it both into one. 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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "umesh9794 (via GitHub)" <gi...@apache.org>.
umesh9794 commented on code in PR #10549:
URL: https://github.com/apache/ozone/pull/10549#discussion_r3542752774
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -227,6 +291,43 @@ private int deleteScmStatesForContainers(DSLContext dslContext,
return totalDeleted;
}
+ private void deleteStaleUnhealthyRecords(DSLContext dslContext,
Review Comment:
Yes, will cover this too
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on code in PR #10549:
URL: https://github.com/apache/ozone/pull/10549#discussion_r3528281318
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -227,6 +291,43 @@ private int deleteScmStatesForContainers(DSLContext dslContext,
return totalDeleted;
}
+ private void deleteStaleUnhealthyRecords(DSLContext dslContext,
Review Comment:
This impl also not doing any batching.
##########
hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/persistence/TestContainerHealthSchemaManager.java:
##########
@@ -0,0 +1,200 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.recon.persistence;
+
+import static org.apache.ozone.recon.schema.generated.tables.UnhealthyContainersTable.UNHEALTHY_CONTAINERS;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.ozone.recon.persistence.ContainerHealthSchemaManager.ContainerStateKey;
+import org.apache.hadoop.ozone.recon.persistence.ContainerHealthSchemaManager.UnhealthyContainerRecord;
+import org.apache.ozone.recon.schema.ContainerSchemaDefinition;
+import org.apache.ozone.recon.schema.ContainerSchemaDefinition.UnHealthyContainerStates;
+import org.jooq.DSLContext;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Functional tests for {@link ContainerHealthSchemaManager#syncUnhealthyContainerRecordsAtomically}.
+ */
+public class TestContainerHealthSchemaManager extends AbstractReconSqlDBTest {
+
+ private static final long ORIGINAL_TIMESTAMP = 1_000L;
+ private static final long UPDATED_TIMESTAMP = 2_000L;
+
+ private ContainerSchemaDefinition schemaDefinition;
+ private ContainerHealthSchemaManager schemaManager;
+
+ @BeforeEach
+ public void setUpSchemaManager() {
+ schemaDefinition = getSchemaDefinition(ContainerSchemaDefinition.class);
+ schemaManager =
+ new ContainerHealthSchemaManager(schemaDefinition, new OzoneConfiguration());
+ }
+
+ @Test
+ public void testSyncInsertsNewUnhealthyRecords() {
+ Map<ContainerStateKey, Long> existing = Collections.emptyMap();
+ List<UnhealthyContainerRecord> desired = Arrays.asList(
+ record(1L, UnHealthyContainerStates.MISSING, ORIGINAL_TIMESTAMP, 3, 0),
+ record(2L, UnHealthyContainerStates.UNDER_REPLICATED, ORIGINAL_TIMESTAMP, 3, 2));
+
+ schemaManager.syncUnhealthyContainerRecordsAtomically(existing, desired);
+
+ assertEquals(Arrays.asList(1L, 2L), remainingContainerIds());
+ assertEquals(2L, countRows());
+ }
+
+ @Test
+ public void testSyncDeletesStaleRowsWhenContainerBecomesHealthy() {
+ insert(record(1L, UnHealthyContainerStates.MISSING, ORIGINAL_TIMESTAMP, 3, 0));
+ insert(record(1L, UnHealthyContainerStates.UNDER_REPLICATED, ORIGINAL_TIMESTAMP, 3, 2));
+ insert(record(2L, UnHealthyContainerStates.MISSING, ORIGINAL_TIMESTAMP, 3, 0));
+
+ Map<ContainerStateKey, Long> existing = existingMap(
+ key(1L, UnHealthyContainerStates.MISSING),
+ key(1L, UnHealthyContainerStates.UNDER_REPLICATED),
+ key(2L, UnHealthyContainerStates.MISSING));
+
+ // Container 1 recovered; container 2 still missing.
+ List<UnhealthyContainerRecord> desired = Collections.singletonList(
+ record(2L, UnHealthyContainerStates.MISSING, ORIGINAL_TIMESTAMP, 3, 0));
+
+ schemaManager.syncUnhealthyContainerRecordsAtomically(existing, desired);
+
+ assertEquals(Collections.singletonList(2L), remainingContainerIds());
+ assertEquals(1L, countRows());
+ }
+
+ @Test
+ public void testSyncUpdatesExistingRowAndDeletesChangedState() {
+ insert(record(1L, UnHealthyContainerStates.MISSING, ORIGINAL_TIMESTAMP, 3, 0));
+
+ Map<ContainerStateKey, Long> existing = existingMap(
+ key(1L, UnHealthyContainerStates.MISSING));
+
+ // Same container now under-replicated instead of missing.
+ List<UnhealthyContainerRecord> desired = Collections.singletonList(
+ record(1L, UnHealthyContainerStates.UNDER_REPLICATED, UPDATED_TIMESTAMP, 3, 2));
+
+ schemaManager.syncUnhealthyContainerRecordsAtomically(existing, desired);
+
+ assertEquals(Collections.singletonList(1L), remainingContainerIds());
+ assertEquals(1L, countRows());
+ UnhealthyContainerRecord row = fetchRow(1L, UnHealthyContainerStates.UNDER_REPLICATED);
+ assertEquals(UPDATED_TIMESTAMP, row.getInStateSince());
+ assertEquals(2, row.getActualReplicaCount());
+ assertTrue(countByState(UnHealthyContainerStates.MISSING) == 0);
+ }
+
+ @Test
+ public void testSyncUpdatesExistingRowInPlace() {
+ insert(record(1L, UnHealthyContainerStates.UNDER_REPLICATED,
+ ORIGINAL_TIMESTAMP, 3, 2, "old reason"));
+
+ Map<ContainerStateKey, Long> existing = existingMap(
+ key(1L, UnHealthyContainerStates.UNDER_REPLICATED));
+
+ List<UnhealthyContainerRecord> desired = Collections.singletonList(
+ record(1L, UnHealthyContainerStates.UNDER_REPLICATED,
+ ORIGINAL_TIMESTAMP, 3, 1, "new reason"));
+
+ schemaManager.syncUnhealthyContainerRecordsAtomically(existing, desired);
+
+ assertEquals(1L, countRows());
+ UnhealthyContainerRecord row = fetchRow(1L, UnHealthyContainerStates.UNDER_REPLICATED);
+ assertEquals(ORIGINAL_TIMESTAMP, row.getInStateSince());
+ assertEquals(1, row.getActualReplicaCount());
+ assertEquals("new reason", row.getReason());
+ }
+
+ @Test
+ public void testBatchDeleteStillRemovesAllScmStatesForContainer() {
Review Comment:
This test calls `batchDeleteSCMStatesForContainers` — the old delete method. It does not test the new `syncUnhealthyContainerRecordsAtomically` method.
A short test like this would be helpful:
1. Insert an `ALL_REPLICAS_BAD` row for container 1 (a non-SCM state).
2. Also insert, say, a `MISSING` row for container 1 (an SCM state) and put it in the "existing" map.
3. Run `syncUnhealthyContainerRecordsAtomically` with a "desired" result where container 1 is no longer `MISSING` (so the SCM row should be deleted).
4. Assert that the `MISSING` row is gone but the `ALL_REPLICAS_BAD` row is still there.
That directly proves, against the real production method, "sync cleans up SCM states but never harms non-SCM states."
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -179,6 +179,70 @@ public void batchDeleteSCMStatesForContainers(List<Long> containerIds) {
totalDeleted, containerIds.size());
}
+ /**
+ * Atomically syncs unhealthy rows for a scan chunk: insert new
+ * (container_id, state) pairs, update existing pairs, and delete only stale
+ * pairs that are no longer unhealthy.
+ *
+ * <p>Unlike {@link #replaceUnhealthyContainerRecordsAtomically}, this does not
+ * delete all SCM states for every container that previously had a row. It
+ * removes only keys present in {@code existingByKey} but absent from the
+ * desired scan result (containers that recovered or changed state).</p>
+ *
+ * @param existingByKey prior rows for this chunk, keyed by
+ * (container_id, container_state)
+ * @param desiredRecords unhealthy rows produced by the current scan
+ */
+ public void syncUnhealthyContainerRecordsAtomically(
+ Map<ContainerStateKey, Long> existingByKey,
+ List<UnhealthyContainerRecord> desiredRecords) {
+ if ((existingByKey == null || existingByKey.isEmpty())
+ && (desiredRecords == null || desiredRecords.isEmpty())) {
+ return;
+ }
+
+ Map<ContainerStateKey, UnhealthyContainerRecord> desiredByKey = new HashMap<>();
+ if (desiredRecords != null) {
+ for (UnhealthyContainerRecord record : desiredRecords) {
+ desiredByKey.put(new ContainerStateKey(record.getContainerId(),
+ record.getContainerState()), record);
+ }
+ }
+
+ Map<ContainerStateKey, Long> existing =
+ existingByKey == null ? new HashMap<>() : existingByKey;
+
+ List<ContainerStateKey> staleKeys = new ArrayList<>();
+ List<UnhealthyContainerRecord> toInsert = new ArrayList<>();
+ List<UnhealthyContainerRecord> toUpdate = new ArrayList<>();
+
+ for (ContainerStateKey key : existing.keySet()) {
+ if (!desiredByKey.containsKey(key)) {
+ staleKeys.add(key);
+ }
+ }
+ for (UnhealthyContainerRecord record : desiredByKey.values()) {
+ ContainerStateKey key = new ContainerStateKey(record.getContainerId(),
+ record.getContainerState());
+ if (existing.containsKey(key)) {
+ toUpdate.add(record);
+ } else {
+ toInsert.add(record);
+ }
+ }
+
+ DSLContext dslContext = containerSchemaDefinition.getDSLContext();
+ dslContext.transaction(configuration -> {
+ DSLContext txContext = configuration.dsl();
+ deleteStaleUnhealthyRecords(txContext, staleKeys);
+ batchInsertInChunks(txContext, toInsert);
+ batchUpdateInChunks(txContext, toUpdate);
Review Comment:
I think , here this is not determining real rows to be updated. Only by checking if row already exists doesn't necessarily required to be updated. Better to identify row to be updated if row content really changed.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -227,6 +291,43 @@ private int deleteScmStatesForContainers(DSLContext dslContext,
return totalDeleted;
}
+ private void deleteStaleUnhealthyRecords(DSLContext dslContext,
+ List<ContainerStateKey> staleKeys) {
+ if (staleKeys.isEmpty()) {
+ return;
+ }
+ for (ContainerStateKey key : staleKeys) {
+ dslContext.deleteFrom(UNHEALTHY_CONTAINERS)
+ .where(UNHEALTHY_CONTAINERS.CONTAINER_ID.eq(key.getContainerId()))
+ .and(UNHEALTHY_CONTAINERS.CONTAINER_STATE.eq(key.getContainerState()))
+ .execute();
+ }
+ }
+
+ private void batchUpdateInChunks(DSLContext dslContext,
+ List<UnhealthyContainerRecord> recs) {
+ if (recs.isEmpty()) {
+ return;
+ }
+ for (int from = 0; from < recs.size(); from += BATCH_INSERT_CHUNK_SIZE) {
Review Comment:
This impl doesn't seem to doing batch update.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "umesh9794 (via GitHub)" <gi...@apache.org>.
umesh9794 commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-4957119344
> @umesh9794 thanks for addressing some concerns in your latest commit, however below points are still open:
>
> 1. remove the dead code `replaceUnhealthyContainerRecordsAtomically`
> 2. I still don't see the real performance comparison as per my comment in earlier review iteration.File based derby and memory based run performance figures are not the real comparison.
Thanks, I removed the dead code.
The earlier shared performance comparisons were between different runs with in-memory derby only and not the file based derby. Please refer below table :
| Run | Code Version | DB |
|--------|--------|--------|
| 82s | [Cell](master: delete-all + re-insert) | in-memory Derby |
| 18s | Diff based sync | in-memory Derby |
| 17.5s | diff-based sync + JDBC batching | in-memory Derby |
I can see production Recon uses file-based Derby, not in-memory. So these absolute numbers are optimistic — real production timings would be higher because of disk I/O and fsync that this benchmark deliberately removes.
Because it's in-memory (and even file-based Derby is local/embedded, not networked), the JDBC batching win is understated in these numbers. Batching mainly saves round-trips IMO; there are none to save in-memory. On file-based Derby, batching (fewer commits/statement executions against disk) could show a somewhat larger benefit than the ~18s→17.5s seen here — but the dominant real-world win can be the change-detection skip of unchanged rows. What do you think?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "umesh9794 (via GitHub)" <gi...@apache.org>.
umesh9794 commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-4764768371
> Thanks @umesh9794 for the patch. Next time please keep the original PR instead of opening a new one. Now discussion will be split between two PRs.
I agree, actually that was my mistake. I reverted the commit and pushed in the same branch which accidentally closed the PR which was unintentional.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #10549:
URL: https://github.com/apache/ozone/pull/10549#discussion_r3442269532
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -205,28 +223,76 @@ public void replaceUnhealthyContainerRecordsAtomically(
private int deleteScmStatesForContainers(DSLContext dslContext,
List<Long> containerIds) {
+
+ // Sort and de-duplicate so contiguous runs are detectable regardless of the
+ // order in which the caller supplied the ids.
+ List<Long> sortedIds = containerIds.stream()
+ .distinct()
+ .sorted()
+ .collect(Collectors.toList());
+
int totalDeleted = 0;
+ List<Long> inClauseBatch = new ArrayList<>(MAX_IN_CLAUSE_CHUNK_SIZE);
+
+ int i = 0;
+ while (i < sortedIds.size()) {
+ int runStart = i;
+ while (i + 1 < sortedIds.size()
+ && sortedIds.get(i + 1) == sortedIds.get(i) + 1) {
+ i++;
+ }
- for (int from = 0; from < containerIds.size(); from += MAX_IN_CLAUSE_CHUNK_SIZE) {
- int to = Math.min(from + MAX_IN_CLAUSE_CHUNK_SIZE, containerIds.size());
- List<Long> chunk = containerIds.subList(from, to);
-
- int deleted = dslContext.deleteFrom(UNHEALTHY_CONTAINERS)
- .where(UNHEALTHY_CONTAINERS.CONTAINER_ID.in(chunk))
- .and(UNHEALTHY_CONTAINERS.CONTAINER_STATE.in(
- UnHealthyContainerStates.MISSING.toString(),
- UnHealthyContainerStates.EMPTY_MISSING.toString(),
- UnHealthyContainerStates.UNDER_REPLICATED.toString(),
- UnHealthyContainerStates.OVER_REPLICATED.toString(),
- UnHealthyContainerStates.MIS_REPLICATED.toString(),
- UnHealthyContainerStates.NEGATIVE_SIZE.toString(),
- UnHealthyContainerStates.REPLICA_MISMATCH.toString()))
- .execute();
- totalDeleted += deleted;
+ if (i - runStart + 1 >= MIN_RANGE_DELETE_RUN) {
+ // Long contiguous run: a single constant-size BETWEEN removes the whole
+ // range without straining Derby's per-statement bytecode budget.
+ totalDeleted += deleteScmStatesByRange(dslContext,
+ sortedIds.get(runStart), sortedIds.get(i));
Review Comment:
What about the suggestion to include multiple `BETWEEN` ranges in the same delete statement?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-4971990476
> > @umesh9794 thanks for addressing some concerns in your latest commit, however below points are still open:
> >
> > 1. remove the dead code `replaceUnhealthyContainerRecordsAtomically`
> > 2. I still don't see the real performance comparison as per my comment in earlier review iteration.File based derby and memory based run performance figures are not the real comparison.
>
> Thanks, I removed the dead code.
>
> The earlier shared performance comparisons were between different runs with in-memory derby only and not the file based derby. Please refer below table :
> Run Code Version DB
> 82s [Cell](master: delete-all + re-insert) in-memory Derby
> 18s Diff based sync in-memory Derby
> 17.5s diff-based sync + JDBC batching in-memory Derby
>
> I can see production Recon uses file-based Derby, not in-memory. So these absolute numbers are optimistic — real production timings would be higher because of disk I/O and fsync that this benchmark deliberately removes.
>
> Because it's in-memory (and even file-based Derby is local/embedded, not networked), the JDBC batching win is understated in these numbers. Batching mainly saves round-trips IMO; there are none to save in-memory. On file-based Derby, batching (fewer commits/statement executions against disk) could show a somewhat larger benefit than the ~18s→17.5s seen here — but the dominant real-world win can be the change-detection skip of unchanged rows. What do you think?
@umesh9794 As I stated in my earlier comment, this is not a real comparison , as `82s` was based on disk/file based derby, and 18s or 17.5s is again was measured in memory DB right ?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-5001863059
> Hi @devmadhuu, sure, I have updated the comment which mentioned that in-memory benchmark with file-based derby's benchmark.
>
> Here is the benchmark run for 50K results :
>
> Same scenario for both: 50,000 tracked unhealthy containers, one steady-state scan with ~10% churn (≈3.5K changed, ≈1.5K recovered, ≈45K unchanged). Only the write operation is timed (both approaches read existing rows first in production).
> Before (master: delete-all + re-insert) After (this PR: diff sync)
> Row operations delete 50,000 + insert 48,500 (~98.5K) update 3,500 + delete 1,500 (~5K)
> Time (file-based) ~4.7 s ~3.9 s
>
> So at a realistic 50K chunk the PR is ~17% faster (4.7 s → 3.9 s).
Now the question is , is it really worth to do this effort and PR just for 17% perf improvement. We usually vouch for min 40% perf improvement for any considerable code change.
cc: @jojochuang @smengcl @rakeshadr @adoroszlai
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "umesh9794 (via GitHub)" <gi...@apache.org>.
umesh9794 commented on code in PR #10549:
URL: https://github.com/apache/ozone/pull/10549#discussion_r3450993315
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -205,28 +223,76 @@ public void replaceUnhealthyContainerRecordsAtomically(
private int deleteScmStatesForContainers(DSLContext dslContext,
List<Long> containerIds) {
+
+ // Sort and de-duplicate so contiguous runs are detectable regardless of the
+ // order in which the caller supplied the ids.
+ List<Long> sortedIds = containerIds.stream()
+ .distinct()
+ .sorted()
+ .collect(Collectors.toList());
+
int totalDeleted = 0;
+ List<Long> inClauseBatch = new ArrayList<>(MAX_IN_CLAUSE_CHUNK_SIZE);
+
+ int i = 0;
+ while (i < sortedIds.size()) {
+ int runStart = i;
+ while (i + 1 < sortedIds.size()
+ && sortedIds.get(i + 1) == sortedIds.get(i) + 1) {
+ i++;
+ }
- for (int from = 0; from < containerIds.size(); from += MAX_IN_CLAUSE_CHUNK_SIZE) {
- int to = Math.min(from + MAX_IN_CLAUSE_CHUNK_SIZE, containerIds.size());
- List<Long> chunk = containerIds.subList(from, to);
-
- int deleted = dslContext.deleteFrom(UNHEALTHY_CONTAINERS)
- .where(UNHEALTHY_CONTAINERS.CONTAINER_ID.in(chunk))
- .and(UNHEALTHY_CONTAINERS.CONTAINER_STATE.in(
- UnHealthyContainerStates.MISSING.toString(),
- UnHealthyContainerStates.EMPTY_MISSING.toString(),
- UnHealthyContainerStates.UNDER_REPLICATED.toString(),
- UnHealthyContainerStates.OVER_REPLICATED.toString(),
- UnHealthyContainerStates.MIS_REPLICATED.toString(),
- UnHealthyContainerStates.NEGATIVE_SIZE.toString(),
- UnHealthyContainerStates.REPLICA_MISMATCH.toString()))
- .execute();
- totalDeleted += deleted;
+ if (i - runStart + 1 >= MIN_RANGE_DELETE_RUN) {
+ // Long contiguous run: a single constant-size BETWEEN removes the whole
+ // range without straining Derby's per-statement bytecode budget.
+ totalDeleted += deleteScmStatesByRange(dslContext,
+ sortedIds.get(runStart), sortedIds.get(i));
Review Comment:
Sure, let me apply that change here, 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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "umesh9794 (via GitHub)" <gi...@apache.org>.
umesh9794 commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-5001765271
Hi @devmadhuu, sure, I have updated the comment which mentioned that in-memory benchmark with file-based derby's benchmark.
Here is the benchmark run for 50K results :
Same scenario for both: 50,000 tracked unhealthy containers, one steady-state scan with ~10% churn (≈3.5K changed, ≈1.5K recovered, ≈45K unchanged). Only the write operation is timed (both approaches read existing rows first in production).
| Before (master: delete-all + re-insert) | After (this PR: diff sync)
-- | -- | --
Row operations | delete 50,000 + insert 48,500 (~98.5K) | update 3,500 + delete 1,500 (~5K)
Time (file-based) | ~4.7 s | ~3.9 s
So at a realistic 50K chunk the PR is ~17% faster (4.7 s → 3.9 s).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-5000167079
> @devmadhuu I have added a file based derby test to compare master with this PR with file based derby, here are the outcomes :
>
> **Test 7 (worst case: 1M rows rewritten, single transaction)**
> In-memory vs File based Derby master (delete-all + insert) this PR (diff sync → all updates)
> In-memory ~82 s ~18 s
> File-based ~87 s >300 s → timed out
>
> **Full file-based comparison**
> Test Operation master this PR
> 1 INSERT 1M ~19 s ~17.6 s
> 2 COUNT(*) 1M ~0.27 s ~0.24 s
> 5 Paginated read 200K ~0.55 s ~0.53 s
> 6 Full 1M read ~1.9 s ~1.9 s
> 7 Rewrite 1M, one tx ~87 s >300 s (timeout)
>
> **In-memory:** no log flush, so 1M in-place UPDATEs (~18 s) beat master's bulk delete + insert (~82 s).
>
> **File-based:** the PR's Test 7 issues 1M single-row UPDATEs inside one transaction — each is an index seek + in-place modify + WAL entry, and nothing commits until the end. On disk that's dramatically more expensive than master's bulk delete + append-style insert (which Derby handles efficiently). Hence >300 s vs 87 s.
>
> But Test 7 is not reflects the production scenario IIUC. Production syncs ≤50k-container chunks (not 200k-in-one-transaction), and in steady state most rows are unchanged. The realistic measure is the 50k steady-state chunk:
>
> this PR: ~3.9 s (skips ~45k, writes ~5k) master would unconditionally delete + re-insert the entire working set every scan, regardless of churn.
>
> So IMO the PR is a clear win in the real steady-state pattern, and a regression only in the artificial all-churn-in-one-giant-transaction case.
Thanks @umesh9794 for running the comparison. Please update the PR description / earlier comments. The original "82 s → 18 s, ~4.5× faster" line was an in-memory-only comparison and is misleading now that we have file-based numbers. Let's replace it with the same-backend results so no one reads the old figure. But it is worth to test with atleast 50K if not 200k and publish the results comparison.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-4799282450
@umesh9794 current solution is not technically correct from production perspective. I have mentioned the reason why die to derby constraints. May be you need to think in another direction for solution where we should avoid full deletes of all records and can we bring updates to existing records and delete only those containers which gone back as Healthy. If we want to improve performance, I could foresee this is the the only safest alternative as of now.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-4750961936
Thanks @umesh9794 for the patch. Next time please keep the original PR instead of opening a new one. Now discussion will be split between two PRs.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "devmadhuu (via GitHub)" <gi...@apache.org>.
devmadhuu commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-4892049656
> Also `ContainerHealthSchemaManager.replaceUnhealthyContainerRecordsAtomically` is not used anywhere after this. Should I remove that method?
Any unused code can be removed, but before that make sure that your current solution works well.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "umesh9794 (via GitHub)" <gi...@apache.org>.
umesh9794 commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-4750862732
@devmadhuu @adoroszlai please take a look at this PR. Its part of changes suggested in #10532 10532
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "umesh9794 (via GitHub)" <gi...@apache.org>.
umesh9794 commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-4798427713
Thanks @devmadhuu for reviewing this.
> Earlier old code was safely putting 1000 Ids under IN clause, so it was tested and known that it will not bring Derby's
> 64KB limit issue as all 1000 Ids were used to stay same in count and predicted with size of compiled Derby byte code. > I mean , it was assuring that keep each SQL command small enough that Derby's secret program stays under 64 KB. > Question is now is this new logic brings that safety when at run time it will determine the number of container ids with > mix n match of IN and BETWEEN ? I have a doubt about that and in that case Derby may crash.
For fully scattered ids (no run ≥ 3), the new path degrades to the same IN-only, 1000-id chunks as before. The operand budget uses the same proven cap. The one gap is many small OR'd BETWEEN terms — Its correct that we have not re-measured bytecode for that shape; we can add a cap or compile test if you want a hard guarantee.
> The new derby statement formed in a more complicated way: Lets take an analogy: "find everyone in the range 1–100, > OR everyone in range 150–2000, OR these specific scattered container ids." When a database sees a question full of > "OR ... OR ... OR," it sometimes gives up on using the index and instead just reads the entire table from row to row to > be safe. That's called a full table scan, and it's much slower.
deletes still target container_id (+ fixed 7-state filter) on a small Recon metadata table inside a scheduled task, not a
high-concurrency path. Fewer statements vs many chunked IN deletes is the intended tradeoff.
> the whole optimization only helps when IDs are next to each other (like 100, 101, 102, 103...). That's when you can
> collapse them into a tidy range.
> But in real life, unhealthy container IDs are usually scattered (like 14, 88, 230, 911...). The PR's own description even
> admits it's aiming at "fragmented" data. Here's the irony: scattered data has no runs to collapse. So the clever new
> logic finds nothing to optimize and falls back to behaving like the old code anyway. So in the common case, you've
> added cost and effort for zero benefit. If you have real production kind of test which can prove with figures, please
> publish those stats.
Delete ids come from ordered 50k slices of getContainers() with prior unhealthy rows, not random cluster-wide ids.
Local contiguity is common I believe; pure scattered ids falls back to old `IN` behavior.
> The old version of this logic was about 15 lines and dead simple: "chop the list into groups of 1,000 and delete each
> group." Boring, but easy to read, easy to trust, and it already fully solved the only real requirement (stay under
> Derby's limit).
The old loop was easy to trust because it did one thing: cap IN at 1,000. It was also slow enough that `TestUnhealthyContainersDerbyPerformance` measured ~82s for a single atomic replace of 200k ids — the exact pattern `replaceUnhealthyContainerRecordsAtomically` uses every scan cycle for large chunks.
The added complexity in the new code is bounded to one private method, covered by dedicated tests, and only changes how delete predicates are formed; however semantics (which rows are removed, 7-state filter, transaction boundary) are unchanged.
But if you still think the production will behave differently, please let me know what should I change in this PR?
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "umesh9794 (via GitHub)" <gi...@apache.org>.
umesh9794 commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-4807043992
> @umesh9794 current solution is not technically correct from production perspective. I have mentioned the reason why die to derby constraints. May be you need to think in another direction for solution where we should avoid full deletes of all records and can we bring updates to existing records and delete only those containers which gone back as Healthy. If we want to improve performance, I could foresee this is the the only safest alternative as of now.
Sure @devmadhuu, I have updated the PR where Recon’s unhealthy-container sync no longer deletes all SCM state rows for every tracked container on each scan. Instead, it now diffs against what is already in the DB: insert new unhealthy rows, update rows that are still unhealthy, and delete only rows that are no longer unhealthy (for example, when a container goes back to Healthy or changes state).
This avoids large bulk deletes and re-inserts, keeps Derby safe from oversized SQL predicates, and makes steady-state sync much faster when most containers stay unhealthy across cycles.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "umesh9794 (via GitHub)" <gi...@apache.org>.
umesh9794 commented on PR #10549:
URL: https://github.com/apache/ozone/pull/10549#issuecomment-4808270588
Also `ContainerHealthSchemaManager.replaceUnhealthyContainerRecordsAtomically` is not used anywhere after this. Should I remove that method?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15601. Coalesce contiguous container IDs into range deletes in ContainerHealthSchemaManager [ozone]
Posted by "umesh9794 (via GitHub)" <gi...@apache.org>.
umesh9794 commented on code in PR #10549:
URL: https://github.com/apache/ozone/pull/10549#discussion_r3542755105
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -179,6 +179,70 @@ public void batchDeleteSCMStatesForContainers(List<Long> containerIds) {
totalDeleted, containerIds.size());
}
+ /**
+ * Atomically syncs unhealthy rows for a scan chunk: insert new
+ * (container_id, state) pairs, update existing pairs, and delete only stale
+ * pairs that are no longer unhealthy.
+ *
+ * <p>Unlike {@link #replaceUnhealthyContainerRecordsAtomically}, this does not
+ * delete all SCM states for every container that previously had a row. It
+ * removes only keys present in {@code existingByKey} but absent from the
+ * desired scan result (containers that recovered or changed state).</p>
+ *
+ * @param existingByKey prior rows for this chunk, keyed by
+ * (container_id, container_state)
+ * @param desiredRecords unhealthy rows produced by the current scan
+ */
+ public void syncUnhealthyContainerRecordsAtomically(
+ Map<ContainerStateKey, Long> existingByKey,
+ List<UnhealthyContainerRecord> desiredRecords) {
+ if ((existingByKey == null || existingByKey.isEmpty())
+ && (desiredRecords == null || desiredRecords.isEmpty())) {
+ return;
+ }
+
+ Map<ContainerStateKey, UnhealthyContainerRecord> desiredByKey = new HashMap<>();
+ if (desiredRecords != null) {
+ for (UnhealthyContainerRecord record : desiredRecords) {
+ desiredByKey.put(new ContainerStateKey(record.getContainerId(),
+ record.getContainerState()), record);
+ }
+ }
+
+ Map<ContainerStateKey, Long> existing =
+ existingByKey == null ? new HashMap<>() : existingByKey;
+
+ List<ContainerStateKey> staleKeys = new ArrayList<>();
+ List<UnhealthyContainerRecord> toInsert = new ArrayList<>();
+ List<UnhealthyContainerRecord> toUpdate = new ArrayList<>();
+
+ for (ContainerStateKey key : existing.keySet()) {
+ if (!desiredByKey.containsKey(key)) {
+ staleKeys.add(key);
+ }
+ }
+ for (UnhealthyContainerRecord record : desiredByKey.values()) {
+ ContainerStateKey key = new ContainerStateKey(record.getContainerId(),
+ record.getContainerState());
+ if (existing.containsKey(key)) {
+ toUpdate.add(record);
+ } else {
+ toInsert.add(record);
+ }
+ }
+
+ DSLContext dslContext = containerSchemaDefinition.getDSLContext();
+ dslContext.transaction(configuration -> {
+ DSLContext txContext = configuration.dsl();
+ deleteStaleUnhealthyRecords(txContext, staleKeys);
+ batchInsertInChunks(txContext, toInsert);
+ batchUpdateInChunks(txContext, toUpdate);
Review Comment:
`container_id + container_state` are the key.
`hasSameContent` compares the other five columns (in_state_since, expected_replica_count, actual_replica_count, replica_delta, reason). So change detection covers every non-key column now.
I hope this works better than simply checking the existence of the row for updates.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org