adoroszlai commented on PR #7848: URL: https://github.com/apache/ozone/pull/7848#issuecomment-2659737643
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): https://github.com/apache/ozone/blob/f2d9cc3bb40251e797ef527180b0ad74bfb112fb/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L530-L537 https://github.com/apache/ozone/blob/f2d9cc3bb40251e797ef527180b0ad74bfb112fb/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L4973-L4987 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. https://github.com/apache/ozone/blob/f2d9cc3bb40251e797ef527180b0ad74bfb112fb/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java#L842 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
