Skip to content

Conversation

@adoroszlai
Copy link
Contributor

@adoroszlai adoroszlai commented Feb 10, 2025

What changes were proposed in this pull request?

Create annotated config object OmConfig for Ozone Manager configs. As a first step, move server.list.max.size into it.

This will help reduce boilerplate code required to make properties reconfigurable, which will be useful for speeding up integration tests.

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

How was this patch tested?

CI:
https://github.com/adoroszlai/ozone/actions/runs/13242815738

Copy link
Contributor

@ivandika3 ivandika3 left a comment

Choose a reason for hiding this comment

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

@adoroszlai Thanks for the patch. Left some minor comments.

which will be useful for speeding up integration tests.

Could you help to explain a bit more about how the config object can speed up the integration tests?

@adoroszlai
Copy link
Contributor Author

Thanks @ivandika3 for the review.

Could you help to explain a bit more about how the config object can speed up the integration tests?

The config object itself does not speed up tests.

Currently, if we want to change the value of some configs, we need to restart OM, or create a completely new cluster. Some tests (e.g. TestListKeys) set ozone.om.server.list.max.size = 2, to be able to test key list iterator / paging without creating thousands of keys. Most other tests use the default value (1000). This is a simple setting, does not change any state in OM. We could simply apply the latest setting for each new listKeys call.

Reading the value from OzoneConfiguration each time may be a bit expensive. So we should store it in a member variable. Currently it is stored as a final variable in OzoneManagerRequestHandler. It could be simply made mutable in that class, but this approach has some problems:

  • Have to expose a method to allow changing it.
  • The test needs to know that it is used in OzoneManagerRequestHandler, not in e.g. OzoneManager. It needs to get a reference to the instance.
  • If any other code uses the same config, we need to make these changes there, too. While ozone.om.server.list.max.size is used only in OzoneManagerRequestHandler, other settings are used in multiple places.

Config objects also store references, so we avoid lookup from OzoneConfiguration. Accessors and mutators are natural on these, and an object like OzoneManagerRequestHandler does not need to expose its config. Config objects can also be shared among code that needs to use OM config.

So making the config reference mutable allows setting it without cluster restart, and the test can simply set the desired value in its setup. Of course tests need to be tweaked to do that, speedup does not come automatically.

This can be taken a step further, by making the config property reconfigurable (HDDS-3835), which allows changing the property in real clusters. A callback needs to be registered for each reconfigurable property, which tends to get repetitive (this is the boilerplate I mentioned in the PR description):

reconfigurationHandler =
new ReconfigurationHandler("OM", conf, this::checkAdminUserPrivilege)
.register(OZONE_ADMINISTRATORS, this::reconfOzoneAdmins)
.register(OZONE_READONLY_ADMINISTRATORS,
this::reconfOzoneReadOnlyAdmins)
.register(OZONE_OM_VOLUME_LISTALL_ALLOWED, this::reconfigureAllowListAllVolumes)
.register(OZONE_KEY_DELETING_LIMIT_PER_TASK,
this::reconfOzoneKeyDeletingLimitPerTask);

private String reconfOzoneKeyDeletingLimitPerTask(String newVal) {
Preconditions.checkArgument(Integer.parseInt(newVal) >= 0,
OZONE_KEY_DELETING_LIMIT_PER_TASK + " cannot be negative.");
getConfiguration().set(OZONE_KEY_DELETING_LIMIT_PER_TASK, newVal);
getKeyManager().getDeletingService()
.setKeyLimitPerTask(Integer.parseInt(newVal));
return newVal;
}
private String reconfigureAllowListAllVolumes(String newVal) {
getConfiguration().set(OZONE_OM_VOLUME_LISTALL_ALLOWED, newVal);
setAllowListAllVolumesFromConfig();
return String.valueOf(allowListAllVolumes);
}

Alternatively, a config object can be registered as a whole, and it can handle all its reconfigurable properties (HDDS-8760). See ReplicationManagerConfiguration as an example with many reconfigurable properties.

reconfigurationHandler.register(replicationManager.getConfig());

Copy link
Contributor

@ivandika3 ivandika3 left a comment

Choose a reason for hiding this comment

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

Thank you for the detailed explanation. This is a good direction to take. LGTM +1.

@adoroszlai adoroszlai merged commit 4e7d6b0 into apache:master Feb 15, 2025
43 checks passed
@adoroszlai
Copy link
Contributor Author

Thanks @ivandika3, @smengcl for the review.

@adoroszlai adoroszlai deleted the HDDS-12294 branch February 15, 2025 07:25
aswinshakil added a commit to aswinshakil/ozone that referenced this pull request Feb 19, 2025
Commits:

5b1bf91 HDDS-12361. Mark testGetSnapshotDiffReportJob as flaky
7af621e HDDS-12375. Random object created and used only once (apache#7933)
b274f1f HDDS-12335. Fix ozone admin namespace summary to give complete output (apache#7908)
a0c07c5 HDDS-12364. Require Override annotation for overridden methods (apache#7923)
464cc8e HDDS-11530. Support listMultipartUploads max uploads and markers (apache#7817)
09cb3a4 HDDS-11867. Remove code paths for non-Ratis SCM. (apache#7911)
5788d12 HDDS-12363. Add import options to IntelliJ IDEA style settings (apache#7921)
0759719 HDDS-12215. Mark testContainerStateMachineRestartWithDNChangePipeline as flaky
b84554a HDDS-12331. BlockOutputStream.failedServers is not thread-safe (apache#7885)
11318ee HDDS-12188. Move server-only upgrade classes from hdds-common to hdds-server-framework (apache#7903)
2598941 HDDS-12362. Remove temporary checkstyle suppression file (apache#7920)
fdd05e2 HDDS-12343. Fix spotbugs warnings in Recon (apache#7902)
7ee359d HDDS-12286. Fix license headers and imports for ozone-tools (apache#7919)
e338291 HDDS-12284. Fix license headers and imports for ozone-s3-secret-store (apache#7917)
86e483d HDDS-12275. Fix license headers and imports for ozone-integration-test (apache#7904)
63ef264 HDDS-12164. Rename and deprecate DFSConfigKeysLegacy config keys (apache#7803)
df5d55b HDDS-12283. Fix license headers and imports for ozone-recon-codegen (apache#7916)
92257d2 HDDS-12330. Convert Volume, Bucket and Key count to use comma separated numbers (apache#7881)
9598cbe HDDS-12281. Fix license headers and imports for ozone-filesystem-hadoop3 (apache#7913)
7dc2d30 HDDS-12285. Fix license headers and imports for ozone-s3gateway (apache#7918)
29d772e HDDS-12282. Fix license headers and imports for ozone-recon (apache#7915)
76127ba HDDS-12280. Fix license headers and imports for ozone-filesystem-hadoop2 (apache#7910)
74e7471 HDDS-12279. Fix license headers and imports for ozone-filesystem-common (apache#7909)
6e49e30 HDDS-12278. Fix license headers and imports for ozone-filesystem (apache#7907)
770d3e6 HDDS-12277. Fix license headers and imports for ozone-manager (apache#7906)
df15f4b HDDS-12337. Speed up list tests (apache#7893)
4c367ae HDDS-12276. Fix license headers and imports for ozone-interface-storage (apache#7905)
b7a2ce0 HDDS-12339. Add CI check for PMD (apache#7896)
dda5285 HDDS-12229. Remove Incorrect Warning for OBS Bucket in Namespace CLI Commands apache#7832
70fc8eb HDDS-12329. Specify S3 error for Quota Exceeded (apache#7878)
9d6b692 HDDS-12274. Fix license headers and imports for ozone-insight (apache#7901)
fdaf296 HDDS-12273. Fix license headers and imports for ozone-httpfsgateway (apache#7900)
f565657 HDDS-12272. Fix Fix license headers and imports for mini-chaos-tests. (apache#7899)
7481fe9 HDDS-12271. Fix license headers and imports for ozone-csi (apache#7898)
2f3150a HDDS-12270. Fix license headers and imports for ozone-common (apache#7897)
da6b611 HDDS-12269. Fix license headers and imports for ozone-client (apache#7895)
e93d791 HDDS-12268. Fix license headers and imports for ozone-cli-shell (apache#7894)
12feb40 HDDS-12332. Remove dead code in KeyManagerImpl (apache#7892)
ef0c24f HDDS-12333. Move ozone.om.enable.filesystem.paths into OmConfig (apache#7888)
889c6a3 HDDS-12267. Fix license headers and imports for hdds-tools (apache#7891)
0e109d1 HDDS-12265. Fix license headers and imports for hdds-server-scm (apache#7889)
4762742 HDDS-12266. Fix license headers and imports for hdds-test-utils (apache#7890)
39437ea HDDS-12259. Fix license headers and imports for hdds-container-service (apache#7887)
8bbcb17 HDDS-10760. IOException(String) constructor required for unwrapping from RemoteException (apache#7854)
194077a HDDS-12261. Fix license headers and imports for hdds-server-framework (apache#7886)
feb6cc0 HDDS-12334. Bump zstd-jni to 1.5.6-10 (apache#7884)
4e7d6b0 HDDS-12294. Create config object for OM (apache#7848)
199795c HDDS-12306. OmMetadataManager metrics are always zero (apache#7853)
b912925 HDDS-12257. Fix license headers and imports for hdds-common (apache#7879)
bc16669 HDDS-12311. flaky-test-check split exit code is always 1 (apache#7855)
b97b7dc HDDS-12326. Allow Quasi_Closed to Closed if there is an unhealthy replica <= highest BCSID (apache#7869)
a31d0fb HDDS-12211. Add TestHttpFSMetrics to test OpsCreate and OpsAppend metrics (apache#7860)
f2d9cc3 HDDS-12325. Recon OM DB Incremental update events are not processed correctly. apache#7868
9d2c7a8 HDDS-12264. Fix license headers and imports for rocksdb-checkpoint-differ (apache#7876)
fde72c4 HDDS-11924. Mark testOzoneContainerWithMissingContainer as flaky
e14ab26 HDDS-12313. Disable flaky TestHSync#testConcurrentExceptionHandling
f72631c HDDS-12263. Fix license headers and imports for hdds-rocks-native (apache#7875)
3ef7ed7 HDDS-12262. Fix license headers and imports for hdds-managed-rocksdb (apache#7874)
b192c45 HDDS-12260. Fix license headers and imports for hdds-erasurecode (apache#7873)
62d7723 HDDS-12258. Fix license headers and imports for hdds-config (apache#7872)
10870bb HDDS-12256. Fix license headers and imports for hadoop-hdds/client (apache#7866)
826271e HDDS-12241. Improve error message in CLI for FileSystemException (apache#7864)
f7ed4ee HDDS-12328. Set the log for starting LeakDetector to DEBUG level (apache#7871)
17011de HDDS-12304. Bump GitHub Actions runner to ubuntu-24.04 (apache#7852)
bf456f6 HDDS-12253. Fix checkstyle for hadoop-hdds/annotations (apache#7865)
c7f0cce HDDS-12161. Remove code paths for non-Ratis OM in request/response (apache#7845)
01889f1 HDDS-12031. Enable Ratis by default on an upgraded cluster during SCM start-up. (apache#7831)
819ed25 HDDS-12178. Add direct test-scope dependencies, remove hdds-hadoop-dependency-test (apache#7800)
628cc3b HDDS-10336. Fix SCM BackgroundPipelineCreator for ozone.replication=EC (apache#7750)
48b2ae4 HDDS-12309. Intermittent failure in TestCloseContainerCommandHandler.testThreadPoolPoolSize (apache#7857)
468f7c5 HDDS-12252. New checkstyle for imports and license with suppressions (apache#7836)
ae6c09b HDDS-12302. Fix parameter number warning in SignatureInfo (apache#7863)
28b9a03 HDDS-12312. NodeManager log aggregation to Ozone FileSystem fails. (apache#7856)
7c38331 HDDS-12110. Optimize memory overhead for OM background tasks. (apache#7743)
4efb596 HDDS-12234. Improve error log No leader found when jmx endpoint is accessed (apache#7846)
e414e6d HDDS-10794. Update docker compose user doc (apache#7822)
3baf49f HDDS-12175. Audit logs in SCM shouldn't print delete txns (apache#7805)
3cd0880 HDDS-12245. Share cluster in ACL integration tests (apache#7840)
9f16b37 HDDS-12220. Limit OM/SCM/Recon RocksDB max user log files total size (apache#7847)
c5608ca HDDS-9400. Introduce DatanodeID to avoid passing UUID/String object in SCM (apache#5417)
0a8c03e HDDS-12249. Share cluster in reconfiguration tests (apache#7851)
56d9938 HDDS-12238. Improve OM DELETE_KEY audit log to include key size (apache#7844)
94bda26 HDDS-11953. Improve Recon OM sync process based on continuous pull of OM data. (apache#7810)
2a61f32 HDDS-12291. XceiverClientRatis allows adding ratis data stream configuration (apache#7842)
6f82e7b HDDS-10073. Remove unused code from GenericTestUtils and LambdaTestUtils (apache#7849)
a8d7179 HDDS-11758. Require successful quick checks for repro (apache#7461)
2c92032 HDDS-12201. Remove unused, dead code in httpfsgateway (apache#7814)
fa30000 HDDS-12290. Move custom logic from ci.yml into the check scripts (apache#7841)
743d24d HDDS-12192. Fix TestOzoneShellHA and extract set-bucket-encryption test case (apache#7802)
fdbf76b HDDS-12292. Change log level in SCMNodeManager#getNodesByAddress to debug. (apache#7843)
9f600a7 HDDS-12248. Make allowListAllVolumes reconfigurable in OM (apache#7837)
0447d88 HDDS-12287. Bump sqlite-jdbc to 3.49.0.0 (apache#7839)
bd60be6 HDDS-12205. Reduce log level in TestHSync (apache#7838)
4edd705 HDDS-11784. Allow aborting FSO multipart uploads with missing parent directories (apache#7700)
81709e0 HDDS-12149. Do not require dependency-convergence. (apache#7772)
0acb9ea HDDS-12232. Move container from QUASI_CLODED to CLOSED only when SCM sees all 3 origin node replicas (apache#7834)
013abf3 HDDS-12230. Improve error message in `ozone sh key put` when file not found (apache#7829)
3eecac6 HDDS-12180. Store snapshot in CachingSpaceUsageSource (apache#7798)
c187de0 HDDS-12228. Fix Duplicate Key Violation Condition in FileSizeCountTask. (apache#7824)
60d94cd HDDS-12227. Avoid Clutter in Recon Logs by Reducing Log Level of ContainerSizeCountTask. (apache#7825)
1b5a3bf HDDS-12218. Add more to integration test with shared cluster (apache#7821)
334ad8c HDDS-11866. Remove code paths for non-Ratis OM (apache#7778)
d6bfea0 (apachessh/master) HDDS-12033. ScmHAUnfinalizedStateValidationAction can be remove as it's not used (apache#7820)
371792f HDDS-12231. Logging in Container Balancer is too verbose. (apache#7826)
166e04e HDDS-12217. Remove reference to FileUtil in hdds-common. (apache#7818)
d9c9709 HDDS-12221. Remove unused config property ozone.block.deleting.limit.per.task (apache#7823)
19b96fa HDDS-12044. Fix heatmap calendar closing on skipping years/months (apache#7812)
390ebb9 HDDS-10607. Remove unused config property ozone.block.deleting.container.limit.per.interval (apache#7816)
5bedec0 HDDS-7003. Make read-replicas tool compatible with EC replication type (apache#7528)
06c6a14 HDDS-12159. Remove redundant seek for rocksDBs (apache#7794)
2c92852 HDDS-11442. Add dashboard for memory consumption metrics (apache#7198)

CONFLICTS:

	hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/container/ContainerReplicaInfo.java
	hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/TokenHelper.java
	hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java
	hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/interfaces/Handler.java
	hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/ContainerLogger.java
	hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ec/reconstruction/ECReconstructionCoordinator.java
	hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerCheck.java
	hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
	hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingTask.java
	hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/AbstractBackgroundContainerScanner.java
	hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/BackgroundContainerDataScanner.java
	hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerController.java
	hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OnDemandContainerDataScanner.java
	hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/OzoneContainer.java
	hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/ContainerTestUtils.java
	hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestBlockDeletingService.java
	hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestHddsDispatcher.java
	hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/interfaces/TestHandler.java
	hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainerCheck.java
	hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java
	hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandlerWithUnhealthyContainer.java
	hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/impl/TestFilePerBlockStrategy.java
	hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestBackgroundContainerDataScanner.java
	hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestBackgroundContainerMetadataScanner.java
	hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerScannersAbstract.java
	hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOnDemandContainerDataScanner.java
	hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/replication/TestContainerImporter.java
	hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/replication/TestReplicationSupervisor.java
	hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/protocolPB/StorageContainerLocationProtocolClientSideTranslatorPB.java
	hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeProtocolServer.java
	hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
	hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerReportHandler.java
	hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestIncrementalContainerReportHandler.java
	hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestCloseContainer.java
	hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestOzoneContainerWithTLS.java
	hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestContainerScannerIntegrationAbstract.java
	hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestOnDemandContainerDataScannerIntegration.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants