You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/04/16 12:29:18 UTC
[GitHub] [hadoop-ozone] elek opened a new pull request #834: Hdds 3101.
Depend on lightweight ConfigurationSource interface instead of Hadoop
Configuration
elek opened a new pull request #834: Hdds 3101. Depend on lightweight ConfigurationSource interface instead of Hadoop Configuration
URL: https://github.com/apache/hadoop-ozone/pull/834
## What changes were proposed in this pull request?
To make it possible to create different client jars compiled with different version of Hadoop we need clear and Hadoop independent `hdds-common` (and `hdds-client`) projects.
(For more details about the motivation, check [this](https://lists.apache.org/thread.html/rd0ea00f958368e888db1947eb71e514fb977df0b7baaad928ac50e94%40%3Cozone-dev.hadoop.apache.org%3E) design doc.)
Our current blocker is the usage of `org.apache.hadoop.conf.Configuration`. Configuration class is a heavyweight object from `hadoop-common` which introduce a lot of unnecessary dependencies. It also violates multiple OOP principles, for example the **Dependency inversion** principle (most of the time we don't need all the methods, it would be enough to depend on a reduced set of the methods)
To make our components more independent I propose to depend on a lightweight `ConfigurationSource` interface which includes all the required `getXXX` methods from the existing `Configuration`. OzoneConfiguration can implement that interface (and with older Hadoop we can create direct adapters).
Other motivation: Some of the used helper classes (related to the storage / time units) are not available in Hadoop 2.7. With this approach we can use any old Hadoop configuration as the only required method would be the `.get` method.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3101
## How was this patch tested?
https://github.com/elek/hadoop-ozone/actions/runs/79732132
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request
#834: HDDS-3101. Depend on lightweight ConfigurationSource interface
instead of Hadoop Configuration
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #834: HDDS-3101. Depend on lightweight ConfigurationSource interface instead of Hadoop Configuration
URL: https://github.com/apache/hadoop-ozone/pull/834#discussion_r410162255
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/SpaceUsageCheckFactory.java
##########
@@ -68,14 +69,16 @@ default SpaceUsageCheckFactory setConfiguration(Configuration conf) {
* Defaults to {@link DUFactory} if no class is configured or it cannot be
* instantiated.
*/
- static SpaceUsageCheckFactory create(Configuration config) {
+ static SpaceUsageCheckFactory create(ConfigurationSource config) {
Conf conf = OzoneConfiguration.of(config).getObject(Conf.class);
Class<? extends SpaceUsageCheckFactory> aClass = null;
String className = conf.getClassName();
if (className != null && !className.isEmpty()) {
try {
- aClass = config.getClassByName(className)
- .asSubclass(SpaceUsageCheckFactory.class);
+ aClass =
+ (Class<? extends SpaceUsageCheckFactory>)
Review comment:
Can we keep `.asSubclass(SpaceUsageCheckFactory.class)` instead of the unchecked cast?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request
#834: HDDS-3101. Depend on lightweight ConfigurationSource interface
instead of Hadoop Configuration
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #834: HDDS-3101. Depend on lightweight ConfigurationSource interface instead of Hadoop Configuration
URL: https://github.com/apache/hadoop-ozone/pull/834#discussion_r410160125
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/freon/OzoneGetConf.java
##########
@@ -250,10 +252,13 @@ public int doWorkInternal(OzoneGetConf tool, String[] args)
@Override
public int doWorkInternal(OzoneGetConf tool, String[] args)
throws IOException {
- if (OmUtils.isServiceIdsDefined(tool.getConf())) {
- tool.printOut(OmUtils.getOmHAAddressesById(tool.getConf()).toString());
+ LegacyHadoopConfigurationSource configSource =
+ new LegacyHadoopConfigurationSource(tool.getConf());
Review comment:
Same here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request
#834: HDDS-3101. Depend on lightweight ConfigurationSource interface
instead of Hadoop Configuration
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #834: HDDS-3101. Depend on lightweight ConfigurationSource interface instead of Hadoop Configuration
URL: https://github.com/apache/hadoop-ozone/pull/834#discussion_r410160757
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/fs/ozone/contract/ITestOzoneContractCreate.java
##########
@@ -18,18 +18,19 @@
package org.apache.hadoop.fs.ozone.contract;
+import java.io.IOException;
+
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.contract.AbstractContractCreateTest;
import org.apache.hadoop.fs.contract.AbstractFSContract;
+
import org.junit.AfterClass;
import org.junit.BeforeClass;
-import java.io.IOException;
-
/**
* Ozone contract tests creating files.
*/
-public class ITestOzoneContractCreate extends AbstractContractCreateTest {
+public class ITestOzoneContractCreate extends AbstractContractCreateTest {
Review comment:
Seems unintended.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request
#834: HDDS-3101. Depend on lightweight ConfigurationSource interface
instead of Hadoop Configuration
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #834: HDDS-3101. Depend on lightweight ConfigurationSource interface instead of Hadoop Configuration
URL: https://github.com/apache/hadoop-ozone/pull/834#discussion_r410134994
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
##########
@@ -114,16 +113,20 @@
// A lock that is held during container creation.
private final AutoCloseableLock containerCreationLock;
- public KeyValueHandler(Configuration config, String datanodeId,
+ public KeyValueHandler(ConfigurationSource config, String datanodeId,
ContainerSet contSet, VolumeSet volSet, ContainerMetrics metrics,
Consumer<ContainerReplicaProto> icrSender) {
super(config, datanodeId, contSet, volSet, metrics, icrSender);
containerType = ContainerType.KeyValueContainer;
blockManager = new BlockManagerImpl(config);
chunkManager = ChunkManagerFactory.createChunkManager(config);
- volumeChoosingPolicy = ReflectionUtils.newInstance(conf.getClass(
- HDDS_DATANODE_VOLUME_CHOOSING_POLICY, RoundRobinVolumeChoosingPolicy
- .class, VolumeChoosingPolicy.class), conf);
+ try {
+ volumeChoosingPolicy = conf.getClass(
+ HDDS_DATANODE_VOLUME_CHOOSING_POLICY, RoundRobinVolumeChoosingPolicy
+ .class, VolumeChoosingPolicy.class).newInstance();
Review comment:
I think some custom `VolumeChoosingPolicy` implementation might want to use configuration, which was possible by implementing `Configurable`. Would be nice to retain that (OK to do as a follow-up task).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request
#834: HDDS-3101. Depend on lightweight ConfigurationSource interface
instead of Hadoop Configuration
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #834: HDDS-3101. Depend on lightweight ConfigurationSource interface instead of Hadoop Configuration
URL: https://github.com/apache/hadoop-ozone/pull/834#discussion_r410153324
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/SCMContainerManager.java
##########
@@ -81,11 +81,11 @@
* passed to LevelDB and this memory is allocated in Native code space.
* CacheSize is specified
* in MB.
- * @param conf - {@link Configuration}
+ * @param conf - {@link org.apache.hadoop.hdds.conf.ConfigurationSource}
Review comment:
```suggestion
* @param conf - {@link ConfigurationSource}
```
as it's already imported.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request
#834: HDDS-3101. Depend on lightweight ConfigurationSource interface
instead of Hadoop Configuration
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #834: HDDS-3101. Depend on lightweight ConfigurationSource interface instead of Hadoop Configuration
URL: https://github.com/apache/hadoop-ozone/pull/834#discussion_r410160058
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/freon/OzoneGetConf.java
##########
@@ -234,7 +236,7 @@ public Integer run() throws Exception {
public int doWorkInternal(OzoneGetConf tool, String[] args)
throws IOException {
Collection<InetSocketAddress> addresses = HddsUtils
- .getSCMAddresses(tool.getConf());
+ .getSCMAddresses(new LegacyHadoopConfigurationSource(tool.getConf()));
Review comment:
Wouldn't it be OK to use `OzoneConfiguration.of` instead of `Legacy...` in this class? Especially since we explicitly create `OzoneGetConf` with an `OzoneConfiguration`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on issue #834: HDDS-3101. Depend on
lightweight ConfigurationSource interface instead of Hadoop Configuration
Posted by GitBox <gi...@apache.org>.
elek commented on issue #834: HDDS-3101. Depend on lightweight ConfigurationSource interface instead of Hadoop Configuration
URL: https://github.com/apache/hadoop-ozone/pull/834#issuecomment-615129437
Please review this PR even if it's conflicted. It's a small change, but big patch: I need to rebase it daily.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org