You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "ivanzlenko (via GitHub)" <gi...@apache.org> on 2025/02/25 10:41:20 UTC
[PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
ivanzlenko opened a new pull request, #7968:
URL: https://github.com/apache/ozone/pull/7968
## What changes were proposed in this pull request?
Code for the hdds-container-service made compliant with PMD.FieldDeclarationsShouldBeAtStartOfClass verification rule.
It is done before enabling the rule itself to prevent us reviewing a very huge PR.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-12413
## How was this patch tested?
Patch tested using standard verification pipeline.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r1973059903
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -74,10 +74,13 @@
* log4j.appender.inspectorAppender.layout=org.apache.log4j.PatternLayout
*/
public class KeyValueContainerMetadataInspector implements ContainerInspector {
- public static final Logger LOG =
- LoggerFactory.getLogger(KeyValueContainerMetadataInspector.class);
- public static final Logger REPORT_LOG = LoggerFactory.getLogger(
- "ContainerMetadataInspectorReport");
+ public static final Logger LOG = LoggerFactory.getLogger(KeyValueContainerMetadataInspector.class);
+
+ public static final Logger REPORT_LOG = LoggerFactory.getLogger("ContainerMetadataInspectorReport");
+
+ public static final String SYSTEM_PROPERTY = "ozone.datanode.container.metadata.inspector";
Review Comment:
Related to empty lines: not having one between `class ...` and the first member variable seems odd, especially when adding one between member variables. Example:
https://github.com/apache/ozone/blob/a423b2deaed69640705e6ff006c4d9ab1066c3d4/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/ContainerTestUtils.java#L82-L98
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on PR #7968:
URL: https://github.com/apache/ozone/pull/7968#issuecomment-2812759107
@adoroszlai any chance to have a look?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Move field declarations to start of class in hdds-container-service [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #7968:
URL: https://github.com/apache/ozone/pull/7968#issuecomment-2813945869
I still think formatting changes in `DatanodeConfiguration` are unnecessary, but OK once. Please try to avoid it next time. :)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r1969563628
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -74,10 +74,13 @@
* log4j.appender.inspectorAppender.layout=org.apache.log4j.PatternLayout
*/
public class KeyValueContainerMetadataInspector implements ContainerInspector {
- public static final Logger LOG =
- LoggerFactory.getLogger(KeyValueContainerMetadataInspector.class);
- public static final Logger REPORT_LOG = LoggerFactory.getLogger(
- "ContainerMetadataInspectorReport");
+ public static final Logger LOG = LoggerFactory.getLogger(KeyValueContainerMetadataInspector.class);
+
+ public static final Logger REPORT_LOG = LoggerFactory.getLogger("ContainerMetadataInspectorReport");
+
+ public static final String SYSTEM_PROPERTY = "ozone.datanode.container.metadata.inspector";
Review Comment:
I think we should kind of agree on whether or not to add empty lines between member variables, before starting to change this aspect.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r2049234844
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java:
##########
@@ -627,10 +585,50 @@ public void setWaitOnAllFollowers(boolean val) {
)
private long deleteContainerTimeoutMs = Duration.ofSeconds(60).toMillis();
+ public Duration getBlockDeletionInterval() {
+ return blockDeletionInterval;
+ }
+
+ public void setRecoveringContainerScrubInterval(Duration duration) {
+ recoveringContainerScrubInterval = duration;
+ }
+
+ public Duration getRecoveringContainerScrubInterval() {
+ return recoveringContainerScrubInterval;
+ }
+
+ public void setBlockDeletionInterval(Duration duration) {
+ blockDeletionInterval = duration;
+ }
+
+ public int getBlockDeletionLimit() {
+ return blockLimitPerInterval;
+ }
+
public long getDeleteContainerTimeoutMs() {
return deleteContainerTimeoutMs;
}
+ public Duration getBlockDeletingMaxLockHoldingTime() {
+ return Duration.ofMillis(blockDeletingMaxLockHoldingTime);
+ }
+
+ public void setBlockDeletingMaxLockHoldingTime(Duration maxLockHoldingTime) {
+ blockDeletingMaxLockHoldingTime = maxLockHoldingTime.toMillis();
+ }
+
+ public boolean waitOnAllFollowers() {
+ return waitOnAllFollowers;
+ }
+
+ public void setWaitOnAllFollowers(boolean val) {
+ this.waitOnAllFollowers = val;
+ }
+
+ public void setBlockDeletionLimit(int limit) {
+ this.blockLimitPerInterval = limit;
+ }
Review Comment:
Done
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r2025191892
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/ContainerTestVersionInfo.java:
##########
@@ -50,31 +70,12 @@ public class ContainerTestVersionInfo {
// composite annotation
}
- private static final String[] SCHEMA_VERSIONS = new String[] {
- null,
- OzoneConsts.SCHEMA_V1,
- OzoneConsts.SCHEMA_V2,
- OzoneConsts.SCHEMA_V3,
- };
Review Comment:
Can we merge this PR?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r1969543691
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsPolicyProvider.java:
##########
@@ -36,8 +36,14 @@
@Unstable
public final class HddsPolicyProvider extends PolicyProvider {
- private static final Supplier<HddsPolicyProvider> SUPPLIER =
- MemoizedSupplier.valueOf(HddsPolicyProvider::new);
+ private static final Supplier<HddsPolicyProvider> SUPPLIER = MemoizedSupplier.valueOf(HddsPolicyProvider::new);
Review Comment:
My reasoning here is to improve readability no only in terms of followings checks but fixing general formatting issues. I understand that add additional work in terms of review, but it is not a lot of formatting changes in from my point of view we shall take advantage and clean up our code base as much as possible.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r1969541691
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/ContainerTestVersionInfo.java:
##########
@@ -50,31 +70,12 @@ public class ContainerTestVersionInfo {
// composite annotation
}
- private static final String[] SCHEMA_VERSIONS = new String[] {
- null,
- OzoneConsts.SCHEMA_V1,
- OzoneConsts.SCHEMA_V2,
- OzoneConsts.SCHEMA_V3,
- };
Review Comment:
This is being changed in #7930, let's wait until that one is merged.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r1969796296
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -74,10 +74,13 @@
* log4j.appender.inspectorAppender.layout=org.apache.log4j.PatternLayout
*/
public class KeyValueContainerMetadataInspector implements ContainerInspector {
- public static final Logger LOG =
- LoggerFactory.getLogger(KeyValueContainerMetadataInspector.class);
- public static final Logger REPORT_LOG = LoggerFactory.getLogger(
- "ContainerMetadataInspectorReport");
+ public static final Logger LOG = LoggerFactory.getLogger(KeyValueContainerMetadataInspector.class);
+
+ public static final Logger REPORT_LOG = LoggerFactory.getLogger("ContainerMetadataInspectorReport");
+
+ public static final String SYSTEM_PROPERTY = "ozone.datanode.container.metadata.inspector";
Review Comment:
I'll revert such changes and kick-off discussion on GitHub and mail list in regards to this topic with suggestions how can we enforce one or another approach.
P.S. I'm obviously in favor of a line separator between fields, since typically it far more readable :) Though it varies case by case.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r2025185277
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsPolicyProvider.java:
##########
@@ -36,8 +36,14 @@
@Unstable
public final class HddsPolicyProvider extends PolicyProvider {
- private static final Supplier<HddsPolicyProvider> SUPPLIER =
- MemoizedSupplier.valueOf(HddsPolicyProvider::new);
+ private static final Supplier<HddsPolicyProvider> SUPPLIER = MemoizedSupplier.valueOf(HddsPolicyProvider::new);
Review Comment:
Reverted this line
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r2049241567
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/CopyContainerCompression.java:
##########
@@ -51,18 +51,16 @@ public OutputStream wrap(OutputStream output) {
SNAPPY(CompressorStreamFactory.SNAPPY_FRAMED),
ZSTD(CompressorStreamFactory.ZSTANDARD);
+ private static final Logger LOG = LoggerFactory.getLogger(CopyContainerCompression.class);
+
private final String compressorFactoryName;
+ private static final CopyContainerCompression DEFAULT_COMPRESSION = CopyContainerCompression.NO_COMPRESSION;
Review Comment:
Done
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r2049238524
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/ScmTestMock.java:
##########
@@ -57,6 +57,15 @@ public class ScmTestMock implements StorageContainerDatanodeProtocol {
private String clusterId;
private String scmId;
+ // Map of datanode to containers
+ private Map<DatanodeDetails,
+ Map<String, ContainerReplicaProto>> nodeContainers =
+ new HashMap<>();
Review Comment:
Done
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Move field declarations to start of class in hdds-container-service [ozone]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on PR #7968:
URL: https://github.com/apache/ozone/pull/7968#issuecomment-2814866627
@adoroszlai thanks for the review!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r2049322687
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java:
##########
@@ -627,142 +545,162 @@ public void setWaitOnAllFollowers(boolean val) {
)
private long deleteContainerTimeoutMs = Duration.ofSeconds(60).toMillis();
- public long getDeleteContainerTimeoutMs() {
- return deleteContainerTimeoutMs;
- }
-
@PostConstruct
public void validate() {
Review Comment:
```
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java
548: Method validate length is 159 lines (max allowed is 150).
```
Can we defer improvements in `validate()` to a separate 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.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r2049137118
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/CopyContainerCompression.java:
##########
@@ -51,18 +51,16 @@ public OutputStream wrap(OutputStream output) {
SNAPPY(CompressorStreamFactory.SNAPPY_FRAMED),
ZSTD(CompressorStreamFactory.ZSTANDARD);
+ private static final Logger LOG = LoggerFactory.getLogger(CopyContainerCompression.class);
+
private final String compressorFactoryName;
+ private static final CopyContainerCompression DEFAULT_COMPRESSION = CopyContainerCompression.NO_COMPRESSION;
Review Comment:
I suggest ordering member variables as:
- `static final`
- `static`
- instance variables
I don't know if there is a PMD rule to enforce that, and even if there is, definitely no need to enable it as part of this change. Just make these moves to reduce future change where possible, like in this class.
(Sorry, I forgot to "add comment" in the review.)
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r2049141272
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/CopyContainerCompression.java:
##########
@@ -51,18 +51,16 @@ public OutputStream wrap(OutputStream output) {
SNAPPY(CompressorStreamFactory.SNAPPY_FRAMED),
ZSTD(CompressorStreamFactory.ZSTANDARD);
+ private static final Logger LOG = LoggerFactory.getLogger(CopyContainerCompression.class);
+
private final String compressorFactoryName;
+ private static final CopyContainerCompression DEFAULT_COMPRESSION = CopyContainerCompression.NO_COMPRESSION;
Review Comment:
Yeah, I'll dig on this idea. Seems very reasonable to have either checkstyle or PMD rule which will enforce such order.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r2025191373
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -74,10 +74,13 @@
* log4j.appender.inspectorAppender.layout=org.apache.log4j.PatternLayout
*/
public class KeyValueContainerMetadataInspector implements ContainerInspector {
- public static final Logger LOG =
- LoggerFactory.getLogger(KeyValueContainerMetadataInspector.class);
- public static final Logger REPORT_LOG = LoggerFactory.getLogger(
- "ContainerMetadataInspectorReport");
+ public static final Logger LOG = LoggerFactory.getLogger(KeyValueContainerMetadataInspector.class);
+
+ public static final Logger REPORT_LOG = LoggerFactory.getLogger("ContainerMetadataInspectorReport");
+
+ public static final String SYSTEM_PROPERTY = "ozone.datanode.container.metadata.inspector";
Review Comment:
Fixed some line breaks where it make sense according to original formatting.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r2049352875
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java:
##########
@@ -627,142 +545,162 @@ public void setWaitOnAllFollowers(boolean val) {
)
private long deleteContainerTimeoutMs = Duration.ofSeconds(60).toMillis();
- public long getDeleteContainerTimeoutMs() {
- return deleteContainerTimeoutMs;
- }
-
@PostConstruct
public void validate() {
Review Comment:
Yeah, was overoptimistic with chages. Reverted
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Move field declarations to start of class in hdds-container-service [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #7968:
URL: https://github.com/apache/ozone/pull/7968
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r2025168108
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -74,10 +74,13 @@
* log4j.appender.inspectorAppender.layout=org.apache.log4j.PatternLayout
*/
public class KeyValueContainerMetadataInspector implements ContainerInspector {
- public static final Logger LOG =
- LoggerFactory.getLogger(KeyValueContainerMetadataInspector.class);
- public static final Logger REPORT_LOG = LoggerFactory.getLogger(
- "ContainerMetadataInspectorReport");
+ public static final Logger LOG = LoggerFactory.getLogger(KeyValueContainerMetadataInspector.class);
+
+ public static final Logger REPORT_LOG = LoggerFactory.getLogger("ContainerMetadataInspectorReport");
+
+ public static final String SYSTEM_PROPERTY = "ozone.datanode.container.metadata.inspector";
Review Comment:
Yeah, actually it maybe worth making a rule for this.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r1969560018
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsPolicyProvider.java:
##########
@@ -36,8 +36,14 @@
@Unstable
public final class HddsPolicyProvider extends PolicyProvider {
- private static final Supplier<HddsPolicyProvider> SUPPLIER =
- MemoizedSupplier.valueOf(HddsPolicyProvider::new);
+ private static final Supplier<HddsPolicyProvider> SUPPLIER = MemoizedSupplier.valueOf(HddsPolicyProvider::new);
Review Comment:
I understand. It's just that there was nothing wrong with the original. There will inevitably be some classes with long name where these need to be wrapped anyway.
This one is wrapped badly and I agree should be changed:
```java
private static final Logger LOG = LoggerFactory
.getLogger(XceiverServerRatis.class);
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r2049126271
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/ScmTestMock.java:
##########
@@ -57,6 +57,15 @@ public class ScmTestMock implements StorageContainerDatanodeProtocol {
private String clusterId;
private String scmId;
+ // Map of datanode to containers
+ private Map<DatanodeDetails,
+ Map<String, ContainerReplicaProto>> nodeContainers =
+ new HashMap<>();
Review Comment:
nit: formatting
```suggestion
private Map<DatanodeDetails, Map<String, ContainerReplicaProto>> nodeContainers =
new HashMap<>();
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/DatanodeConfiguration.java:
##########
@@ -627,10 +585,50 @@ public void setWaitOnAllFollowers(boolean val) {
)
private long deleteContainerTimeoutMs = Duration.ofSeconds(60).toMillis();
+ public Duration getBlockDeletionInterval() {
+ return blockDeletionInterval;
+ }
+
+ public void setRecoveringContainerScrubInterval(Duration duration) {
+ recoveringContainerScrubInterval = duration;
+ }
+
+ public Duration getRecoveringContainerScrubInterval() {
+ return recoveringContainerScrubInterval;
+ }
+
+ public void setBlockDeletionInterval(Duration duration) {
+ blockDeletionInterval = duration;
+ }
+
+ public int getBlockDeletionLimit() {
+ return blockLimitPerInterval;
+ }
+
public long getDeleteContainerTimeoutMs() {
return deleteContainerTimeoutMs;
}
+ public Duration getBlockDeletingMaxLockHoldingTime() {
+ return Duration.ofMillis(blockDeletingMaxLockHoldingTime);
+ }
+
+ public void setBlockDeletingMaxLockHoldingTime(Duration maxLockHoldingTime) {
+ blockDeletingMaxLockHoldingTime = maxLockHoldingTime.toMillis();
+ }
+
+ public boolean waitOnAllFollowers() {
+ return waitOnAllFollowers;
+ }
+
+ public void setWaitOnAllFollowers(boolean val) {
+ this.waitOnAllFollowers = val;
+ }
+
+ public void setBlockDeletionLimit(int limit) {
+ this.blockLimitPerInterval = limit;
+ }
Review Comment:
Since we are already moving these, how about putting `get/set` pairs next to each other, in consistent order?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -74,10 +74,13 @@
* log4j.appender.inspectorAppender.layout=org.apache.log4j.PatternLayout
*/
public class KeyValueContainerMetadataInspector implements ContainerInspector {
- private static final Logger LOG =
- LoggerFactory.getLogger(KeyValueContainerMetadataInspector.class);
- public static final Logger REPORT_LOG = LoggerFactory.getLogger(
- "ContainerMetadataInspectorReport");
+ private static final Logger LOG = LoggerFactory.getLogger(KeyValueContainerMetadataInspector.class);
+ public static final Logger REPORT_LOG = LoggerFactory.getLogger("ContainerMetadataInspectorReport");
+
+ public static final String SYSTEM_PROPERTY = "ozone.datanode.container" +
+ ".metadata.inspector";
Review Comment:
I think this can be one line, which will also make grep for the property name turn up this class.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r2049137118
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/CopyContainerCompression.java:
##########
@@ -51,18 +51,16 @@ public OutputStream wrap(OutputStream output) {
SNAPPY(CompressorStreamFactory.SNAPPY_FRAMED),
ZSTD(CompressorStreamFactory.ZSTANDARD);
+ private static final Logger LOG = LoggerFactory.getLogger(CopyContainerCompression.class);
+
private final String compressorFactoryName;
+ private static final CopyContainerCompression DEFAULT_COMPRESSION = CopyContainerCompression.NO_COMPRESSION;
Review Comment:
I suggest ordering member variables as:
- `static final`
- `static`
- instance variables
I don't know if there is a PMD rule to enforce that, and even if there is, definitely no need to enable it as part of this change. Just make these moves to reduce future change where possible, like in this class.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "ivanzlenko (via GitHub)" <gi...@apache.org>.
ivanzlenko commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r2049236359
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainerMetadataInspector.java:
##########
@@ -74,10 +74,13 @@
* log4j.appender.inspectorAppender.layout=org.apache.log4j.PatternLayout
*/
public class KeyValueContainerMetadataInspector implements ContainerInspector {
- private static final Logger LOG =
- LoggerFactory.getLogger(KeyValueContainerMetadataInspector.class);
- public static final Logger REPORT_LOG = LoggerFactory.getLogger(
- "ContainerMetadataInspectorReport");
+ private static final Logger LOG = LoggerFactory.getLogger(KeyValueContainerMetadataInspector.class);
+ public static final Logger REPORT_LOG = LoggerFactory.getLogger("ContainerMetadataInspectorReport");
+
+ public static final String SYSTEM_PROPERTY = "ozone.datanode.container" +
+ ".metadata.inspector";
Review Comment:
Done
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12413. Make hdds-container-service compliant with FieldDeclarationsShouldBeAtStartOfClass [ozone]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #7968:
URL: https://github.com/apache/ozone/pull/7968#discussion_r1969536592
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/HddsPolicyProvider.java:
##########
@@ -36,8 +36,14 @@
@Unstable
public final class HddsPolicyProvider extends PolicyProvider {
- private static final Supplier<HddsPolicyProvider> SUPPLIER =
- MemoizedSupplier.valueOf(HddsPolicyProvider::new);
+ private static final Supplier<HddsPolicyProvider> SUPPLIER = MemoizedSupplier.valueOf(HddsPolicyProvider::new);
Review Comment:
Can we avoid reformat-only changes?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org