You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2022/07/01 06:15:15 UTC
[GitHub] [ratis] szetszwo opened a new pull request, #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
szetszwo opened a new pull request, #667:
URL: https://github.com/apache/ratis/pull/667
See https://issues.apache.org/jira/browse/RATIS-1609
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] captainzmc commented on pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #667:
URL: https://github.com/apache/ratis/pull/667#issuecomment-1259675985
Hi @szetszwo, I am currently testing ratis 2.4.0 in Ozone with @codings-dan. We found a possible compatibility issue.
Here we move [statemachine.Initialize()](https://github.com/apache/ratis/pull/667/files#diff-62ccf21408c07a3fcfe1c3e3f7d5cdff22a8efbcdea9eacae3fe14351c404af3L140) from the constructor to Initialize (), which is executed only in the [start()](https://github.com/apache/ratis/pull/667/files#diff-ad3a60b7e17aef8c1e813168c5ebc591dc7259b2427d8870a95a77c010e5692dR308) of RaftServerImpl.
This changes the execution logic of RaftServer. Such as Ozone testInstallCorruptedCheckpointFailure, original MiniOzoneCluster need to create three SCM and start the 2 sets of them. In the logic of the original three SCMStateMachine is running, Now there are only the first two. This causes the third SCMStateMachine status [check to be incorrect](https://github.com/captainzmc/hadoop-ozone/actions/runs/3098099863/jobs/5015584764#step:6:3147).
Unless we find a way to execute ServerState‘s [initialize(stateMachine)](https://github.com/apache/ratis/pull/667/files#diff-62ccf21408c07a3fcfe1c3e3f7d5cdff22a8efbcdea9eacae3fe14351c404af3R156) in Ozone UT.
In addition to the SCM UT, we also found this problem in the [OM UT](https://github.com/captainzmc/hadoop-ozone/actions/runs/3098099863/jobs/5015584440#step:5:3105). Of course, this is only a problem of UT, we did not find this problem in the actual use of Ozone.
At present, I haven't thought of a good way to fix these UT. Do you have any suggestions?
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] lokeshj1703 commented on a diff in pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
lokeshj1703 commented on code in PR #667:
URL: https://github.com/apache/ratis/pull/667#discussion_r915645955
##########
ratis-server/src/main/java/org/apache/ratis/server/storage/RaftStorageImpl.java:
##########
@@ -38,30 +37,35 @@ public class RaftStorageImpl implements RaftStorage {
// TODO support multiple storage directories
private final RaftStorageDirectoryImpl storageDir;
- private final StorageState state;
+ private final StartupOption startupOption;
private final CorruptionPolicy logCorruptionPolicy;
+ private volatile StorageState state = StorageState.UNINITIALIZED;
private volatile RaftStorageMetadataFileImpl metaFile;
RaftStorageImpl(File dir, CorruptionPolicy logCorruptionPolicy, StartupOption option,
- long storageFeeSpaceMin) throws IOException {
+ long storageFeeSpaceMin) {
this.storageDir = new RaftStorageDirectoryImpl(dir, storageFeeSpaceMin);
- if (option == StartupOption.FORMAT) {
+ this.logCorruptionPolicy = Optional.ofNullable(logCorruptionPolicy).orElseGet(CorruptionPolicy::getDefault);
+ this.startupOption = option;
+ }
+
+ @Override
+ public void initialize() throws IOException {
Review Comment:
We can probably introduce a try catch block here and unlock the storageDir in case of an exception.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] captainzmc commented on pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #667:
URL: https://github.com/apache/ratis/pull/667#issuecomment-1262472832
`could you share the Ratis code being tested with Ozone `
@szetszwo Sure, I used yaolong's private branch. https://github.com/codings-dan/incubator-ratis/commits/2.4.0-rc2-test
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] captainzmc commented on pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #667:
URL: https://github.com/apache/ratis/pull/667#issuecomment-1263228197
By update some Ozone UT, the CI can be executed successfully. @codings-dan, let's start voting for the release of ratis 2.4.0.
https://github.com/captainzmc/hadoop-ozone/actions/runs/3156724171/jobs/5136739190
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #667:
URL: https://github.com/apache/ratis/pull/667#issuecomment-1263284435
@captainzmc , @codings-dan , That's great. Thanks a lot!
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #667:
URL: https://github.com/apache/ratis/pull/667#issuecomment-1262462693
@captainzmc , for https://github.com/captainzmc/hadoop-ozone/actions/runs/3098099863/jobs/5015584764#step:6:3146 , we may check LifeCycle state as below.
```java
+++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMStateMachine.java
@@ -353,8 +353,11 @@ public void notifyConfigurationChanged(long term, long index,
@Override
public void pause() {
- getLifeCycle().transition(LifeCycle.State.PAUSING);
- getLifeCycle().transition(LifeCycle.State.PAUSED);
+ final LifeCycle lc = getLifeCycle();
+ if (lc.getCurrentState() != LifeCycle.State.NEW) {
+ lc.transition(LifeCycle.State.PAUSING);
+ lc.transition(LifeCycle.State.PAUSED);
+ }
}
@Override
```
BTW, could you share the Ratis code being tested with Ozone? I tried to use Ratis branch-2 but got some different errors.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] codings-dan commented on pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
codings-dan commented on PR #667:
URL: https://github.com/apache/ratis/pull/667#issuecomment-1263279417
Thanks @szetszwo and @captainzmc for helping test and fix compatibility issues related to ratis 2.4.0
> let's start voting for the release of ratis 2.4.0.
Sure, will do!
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #667:
URL: https://github.com/apache/ratis/pull/667#issuecomment-1262523137
> ... yaolong's private branch. https://github.com/codings-dan/incubator-ratis/commits/2.4.0-rc2-test
@captainzmc , Thanks. Both TestAddRemoveOzoneManager.testBootstrapWithoutConfigUpdate() and TestSCMInstallSnapshotWithHA.testInstallCorruptedCheckpointFailure() can pass with [this fix](https://github.com/apache/ratis/pull/667#issuecomment-1262462693).
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #667:
URL: https://github.com/apache/ratis/pull/667#discussion_r916171229
##########
ratis-server/src/main/java/org/apache/ratis/server/storage/RaftStorageImpl.java:
##########
@@ -38,30 +37,35 @@ public class RaftStorageImpl implements RaftStorage {
// TODO support multiple storage directories
private final RaftStorageDirectoryImpl storageDir;
- private final StorageState state;
+ private final StartupOption startupOption;
private final CorruptionPolicy logCorruptionPolicy;
+ private volatile StorageState state = StorageState.UNINITIALIZED;
private volatile RaftStorageMetadataFileImpl metaFile;
RaftStorageImpl(File dir, CorruptionPolicy logCorruptionPolicy, StartupOption option,
- long storageFeeSpaceMin) throws IOException {
+ long storageFeeSpaceMin) {
this.storageDir = new RaftStorageDirectoryImpl(dir, storageFeeSpaceMin);
- if (option == StartupOption.FORMAT) {
+ this.logCorruptionPolicy = Optional.ofNullable(logCorruptionPolicy).orElseGet(CorruptionPolicy::getDefault);
+ this.startupOption = option;
+ }
+
+ @Override
+ public void initialize() throws IOException {
Review Comment:
@lokeshj1703 , That's is a good idea! Thanks.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] captainzmc commented on a diff in pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on code in PR #667:
URL: https://github.com/apache/ratis/pull/667#discussion_r981395646
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/ServerState.java:
##########
@@ -137,29 +139,32 @@ class ServerState implements Closeable {
snapshotManager = new SnapshotManager(storage, id);
- stateMachine.initialize(server.getRaftServer(), group.getGroupId(), storage);
Review Comment:
Hi @szetszwo, I am currently testing ratis 2.4.0 in Ozone with @codings-dan. We found a possible compatibility issue.
Here we move statemachine.Initialize() from the constructor to Initialize (), which is executed only in the [start()](https://github.com/apache/ratis/pull/667/files#diff-ad3a60b7e17aef8c1e813168c5ebc591dc7259b2427d8870a95a77c010e5692dR308) of RaftServerImpl.
This changes the execution logic of RaftServer. Such as Ozone testInstallCorruptedCheckpointFailure, original MiniOzoneCluster need to create three SCM and start the 2 sets of them. In the logic of the original three SCMStateMachine is running, Now there are only the first two. This causes the third SCMStateMachine status [check to be incorrect](https://github.com/captainzmc/hadoop-ozone/actions/runs/3098099863/jobs/5015584764#step:6:3147).
Unless we find a way to execute ServerState‘s [initialize(stateMachine)](https://github.com/apache/ratis/pull/667/files#diff-62ccf21408c07a3fcfe1c3e3f7d5cdff22a8efbcdea9eacae3fe14351c404af3R156) in Ozone UT.
In addition to the SCM UT, we also found this problem in the [OM UT](https://github.com/captainzmc/hadoop-ozone/actions/runs/3098099863/jobs/5015584440#step:5:3105). Of course, this is only a problem of UT, we did not find this problem in the actual use of Ozone.
At present, I haven't thought of a good way to fix these UT. Do you have any suggestions?
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #667:
URL: https://github.com/apache/ratis/pull/667#issuecomment-1262465529
> .. call [statemachine.Initialize](https://github.com/apache/ratis/pull/667/files#diff-62ccf21408c07a3fcfe1c3e3f7d5cdff22a8efbcdea9eacae3fe14351c404af3L140) in the ServerState constructor. ...
Since it is only a problem in unit tests, let's try fixing the tests instead changing the main code.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] captainzmc commented on pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #667:
URL: https://github.com/apache/ratis/pull/667#issuecomment-1263065959
Thanks @szetszwo. If we only change Ozone to solve problems, I think it is acceptable. Let me trigger CI to verify that.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #667:
URL: https://github.com/apache/ratis/pull/667#issuecomment-1179143541
@lokeshj1703 , thanks a lot for reviewing 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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo merged pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
szetszwo merged PR #667:
URL: https://github.com/apache/ratis/pull/667
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #667:
URL: https://github.com/apache/ratis/pull/667#issuecomment-1261067520
@captainzmc , thanks for testing ratis 2.4.0. Let me see how to fix the test failures.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #667:
URL: https://github.com/apache/ratis/pull/667#issuecomment-1262530747
I forgot to mention that we also should update the test as below:
```java
+++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/scm/TestSCMInstallSnapshotWithHA.java
import org.junit.jupiter.api.BeforeEach;
@@ -273,7 +274,9 @@ public void testInstallCorruptedCheckpointFailure() throws Exception {
Assert.assertTrue(logCapture.getOutput()
.contains("Failed to reload SCM state and instantiate services."));
- Assert.assertTrue(followerSM.getLifeCycleState().isPausingOrPaused());
+ final LifeCycle.State s = followerSM.getLifeCycleState();
+ Assert.assertTrue("Unexpected lifeCycle state: " + s,
+ s == LifeCycle.State.NEW || s.isPausingOrPaused());
// Verify correct reloading
followerSM.setInstallingDBCheckpoint(
```
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] captainzmc commented on pull request #667: RATIS-1609. RaftStorageImpl should not lock the directory in the constructor.
Posted by GitBox <gi...@apache.org>.
captainzmc commented on PR #667:
URL: https://github.com/apache/ratis/pull/667#issuecomment-1262400818
Hi @szetszwo, I wonder if we can achieve this PR in another way. For example, we still call [statemachine.Initialize](https://github.com/apache/ratis/pull/667/files#diff-62ccf21408c07a3fcfe1c3e3f7d5cdff22a8efbcdea9eacae3fe14351c404af3L140) in the ServerState constructor. However, we can release the lock after using it in RaftStorageImpl's Initialize. At the [start](https://github.com/apache/ratis/pull/667/files#diff-ad3a60b7e17aef8c1e813168c5ebc591dc7259b2427d8870a95a77c010e5692dR304)
we can reacquire the directory lock.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org