HDDS-15601. Improve ContainerHealthSchemaManager delete performance

[umesh9794]

What changes were proposed in this pull request?

Further speed up TestUnhealthyContainersDerbyPerformance by using BETWEEN predicate instead of many IN clauses while deleting the contiguous container IDs

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-15601

How was this patch tested?

Ran the test locally and the elapsed time of this run is significantly reduced from ~313s to ~59s:

[INFO] Running org.apache.hadoop.ozone.recon.persistence.TestUnhealthyContainersDerbyPerformance

[INFO] Tests run: 10, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 58.63 s -- in org.apache.hadoop.ozone.recon.persistence.TestUnhealthyContainersDerbyPerformance

[adoroszlai]

Thanks @umesh9794 for the patch. Please enable the build-branch workflow in your fork.

[adoroszlai]

Further speed up TestUnhealthyContainersDerbyPerformance by using BETWEEN predicate instead of many IN clauses while deleting the contiguous container IDs

While the speed improvement in TestUnhealthyContainersDerbyPerformance is nice (173 -> 51 seconds locally), the test no longer seems to simulate real workload, where a continuous range of 200K containers is unlikely.

[adoroszlai]

@ArafatKhan2198 @devmadhuu ContainerHealthSchemaManager.batchDeleteSCMStatesForContainers is only called from tests, can we delete it?

[umesh9794]

@adoroszlai I have enabled the workflow in my fork. Thanks.

Yes as I was not aware of the codebase very much, this method ContainerHealthSchemaManager.batchDeleteSCMStatesForContainers only gets called from the couple of test classes. Upon conformation of @ArafatKhan2198 and @devmadhuu we can clean-up this probably.

[devmadhuu]

@ArafatKhan2198 @devmadhuu ContainerHealthSchemaManager.batchDeleteSCMStatesForContainers is only called from tests, can we delete it?

Yes, we can remove it as long as our test uses some other real code logic hitting the actual product code and able to test the batch delete performance.

[devmadhuu]

Thanks @umesh9794 for the patch. Given few comments. Pls check.

[devmadhuu]

This below code assumption seems incorrect that in real cluster that the unhealthy container ids all will be in continous sequence.

Real container IDs may not form one continuous sequence.

Consider this input:

1, 2, 4, 5, 7, 8, 10, 11

The PR sees four small continuous ranges and executes:

BETWEEN 1 AND 2
BETWEEN 4 AND 5
BETWEEN 7 AND 8
BETWEEN 10 AND 11

That means four separate DELETE statements.

The old implementation could delete all eight IDs using one statement:

WHERE container_id IN (1, 2, 4, 5, 7, 8, 10, 11)

With a larger realistic list containing many small pairs, the difference could become:

Old code: 50 DELETE statements
New code: 10,000 DELETE statements

Each statement must be compiled and executed by Derby. Consequently, production could become significantly slower even though this test becomes faster.

1, 2, 3, 4, ... 200,000

That is the best possible input for BETWEEN.

It does not test inputs such as:

1, 2, 10, 11, 20, 21, ...

[adoroszlai]

I think we can reduce delete statement count by combining BETWEEN and IN clauses in the same query using OR logic. Delay submitting the statement if number of items for IN reaches the limit or number of BETWEEN ranges reaches another (lower) threshold.

[umesh9794]

@devmadhuu yes current code executes more delete statements and it might slowdown in the real production envs. Let me try to optimize this for production scenarios. Thanks!

[umesh9794]

@adoroszlai sure, trying to check the logic to improve the performance. Thanks!

[adoroszlai]

ContainerHealthSchemaManager.batchDeleteSCMStatesForContainers is only called from tests, can we delete it?

Yes, we can remove it as long as our test uses some other real code logic hitting the actual product code and able to test the batch delete performance.

Thanks @devmadhuu for checking. Yes, product code uses another method:

ozone/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ReconReplicationManager.java

Lines 498 to 504 in fa1b3a3

	private void persistUnhealthyRecords(
	List<Long> containerIdsToDelete,
	List<ContainerHealthSchemaManager.UnhealthyContainerRecord> recordsToInsert) {
	LOG.info("Replacing unhealthy container records atomically: deleteRowsFor={} containers, insert={}",
	containerIdsToDelete.size(), recordsToInsert.size());
	healthSchemaManager.replaceUnhealthyContainerRecordsAtomically(
	containerIdsToDelete, recordsToInsert);

which is also covered by the performance test:

ozone/hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/persistence/TestUnhealthyContainersDerbyPerformance.java

Line 635 in fa1b3a3

schemaManager.replaceUnhealthyContainerRecordsAtomically(idsToReplace, replacementRecords);

So I suggest splitting the task:

1. "Further speed up TestUnhealthyContainersDerbyPerformance": remove test case for batchDeleteSCMStatesForContainers
2. "Improve deleteScmStatesForContainers performance": this PR, needs further work

[devmadhuu]

ContainerHealthSchemaManager.batchDeleteSCMStatesForContainers is only called from tests, can we delete it?

Yes, we can remove it as long as our test uses some other real code logic hitting the actual product code and able to test the batch delete performance.

Thanks @devmadhuu for checking. Yes, product code uses another method:

ozone/hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ReconReplicationManager.java

Lines 498 to 504 in fa1b3a3

	private void persistUnhealthyRecords(
	List<Long> containerIdsToDelete,
	List<ContainerHealthSchemaManager.UnhealthyContainerRecord> recordsToInsert) {
	LOG.info("Replacing unhealthy container records atomically: deleteRowsFor={} containers, insert={}",
	containerIdsToDelete.size(), recordsToInsert.size());
	healthSchemaManager.replaceUnhealthyContainerRecordsAtomically(
	containerIdsToDelete, recordsToInsert);

which is also covered by the performance test:

ozone/hadoop-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/persistence/TestUnhealthyContainersDerbyPerformance.java

Line 635 in fa1b3a3

schemaManager.replaceUnhealthyContainerRecordsAtomically(idsToReplace, replacementRecords);

So I suggest splitting the task:

1. "Further speed up TestUnhealthyContainersDerbyPerformance": remove test case for `batchDeleteSCMStatesForContainers`

2. "Improve deleteScmStatesForContainers performance": this PR, needs further work

Yes. I agree.