-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDDS-1474. ozone.scm.datanode.id config should take path for a dir #792
Conversation
/label ozone |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
/retest |
💔 -1 overall
This message was automatically generated. |
The failing acceptance tests and unit tests are not related to this change. The trunk is broken and these tests need to be fixed in trunk. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vivekratnavel for working on this. LGTM overall. Few minor comments.
...service/src/test/java/org/apache/hadoop/ozone/container/common/TestDatanodeStateMachine.java
Outdated
Show resolved
Hide resolved
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java
Outdated
Show resolved
Hide resolved
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/hdds/scm/HddsServerUtil.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @vivekratnavel .
Just one Nitpick. +1 with that addressed.
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
+1. Thanks @vivekratnavel . Test failures are unrelated. I will merge this shortly. |
@@ -20,7 +20,7 @@ metadata: | |||
name: config | |||
data: | |||
OZONE-SITE.XML_hdds.datanode.dir: /data/storage | |||
OZONE-SITE.XML_ozone.scm.datanode.id: /data/datanode.id | |||
OZONE-SITE.XML_ozone.scm.datanode.id.dir: /data/datanode.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to comment so late, is this wrong ? Shouldn't this be /data instead of /data/datanode.id ?
@@ -20,7 +20,7 @@ metadata: | |||
name: config | |||
data: | |||
OZONE-SITE.XML_hdds.datanode.dir: /data/storage | |||
OZONE-SITE.XML_ozone.scm.datanode.id: /data/datanode.id | |||
OZONE-SITE.XML_ozone.scm.datanode.id.dir: /data/datanode.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same , /data instead of /data/datanode.id?
@@ -20,7 +20,7 @@ metadata: | |||
name: config | |||
data: | |||
OZONE-SITE.XML_hdds.datanode.dir: /data/storage | |||
OZONE-SITE.XML_ozone.scm.datanode.id: /data/datanode.id | |||
OZONE-SITE.XML_ozone.scm.datanode.id.dir: /data/datanode.id | |||
OZONE-SITE.XML_ozone.metadata.dirs: /data/metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here ..thx
Thanks @anuengineer for catching this. I will revert the patch. |
…eckpoint failures. Retry loop in the existing `KafkaCheckpointManager` implementation retries using the same `SystemProducer` instance on exception and does not recreate it. When some irrecoverable exceptions occur within the `SystemProducer`, all the subsequent produce message invocations on the `SystemProducer` instance will fail. This had made the entire retry loop on `KafkaCheckpointManager` pointless. This patch consists of the following changes: 1. This patch addresses the above problem by recreating the `SystemProducer` instance on failure and adds a unit test to verify the functionality. 2. Minor code cleanup in classes: `TestKafkaCheckpointManager` and `KafkaCheckpointManager`. Author: Shanthoosh Venkataraman <[email protected]> Author: Shanthoosh Venkataraman <[email protected]> Reviewers: Dong Lin <[email protected]> Closes apache#792 from shanthoosh/kafka_checkpoint_manager_fix
and not a file.
Currently, the ozone config "ozone.scm.datanode.id" takes file path as its value. It should instead take dir path as its value and assume a standard filename "datanode.id"