Skip to content

RATIS-1588. Add a builder of RaftStorageImpl #652

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.

Sign up for GitHub

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

Jump to bottom
Merged
szetszwo merged 7 commits into from
Jun 17, 2022
Merged

RATIS-1588. Add a builder of RaftStorageImpl #652

szetszwo merged 7 commits into from
Jun 17, 2022

Conversation

@codings-dan
Copy link
Contributor

@codings-dan codings-dan commented Jun 1, 2022

@codings-dan
Copy link
Contributor Author

codings-dan commented Jun 1, 2022

@szetszwo Can you help review this pull request, thanks!

@szetszwo
Copy link
Contributor

szetszwo commented Jun 2, 2022

@codings-dan , sorry that it broke Alluxio. RaftStorageImpl is not a public API so that we do not have to maintain compatibility. We may consider adding a RaftStorage.Builder as a public API.

BTW, this is backward compatibility -- a new version of the software works for the existing applications. Forward compatibility means the current design of the software can work on future applications.

1 similar comment
@szetszwo
Copy link
Contributor

szetszwo commented Jun 2, 2022

@codings-dan , sorry that it broke Alluxio. RaftStorageImpl is not a public API so that we do not have to maintain compatibility. We may consider adding a RaftStorage.Builder as a public API.

BTW, this is backward compatibility -- a new version of the software works for the existing applications. Forward compatibility means the current design of the software can work on future applications.

@codings-dan
Copy link
Contributor Author

codings-dan commented Jun 2, 2022

We may consider adding a RaftStorage.Builder as a public API.

Good idea, I will add a RaftStorage.Builder.

a new version of the software works for the existing applications. Forward compatibility means the current design of the software can work on future applications

Thanks for telling me about these, I now understand the difference!

@codings-dan
Copy link
Contributor Author

codings-dan commented Jun 7, 2022

@szetszwo I have added a RaftStorage.Builder, PTAL again, thx!

@codings-dan codings-dan changed the title RATIS-1588. Add a constructor of RaftStorageImpl for forward compatibility RATIS-1588. Add a builder of RaftStorageImpl Jun 7, 2022
szetszwo
szetszwo reviewed Jun 7, 2022
View reviewed changes
Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codings-dan , thanks for the update! Some comments inlined.

ratis-server/src/main/java/org/apache/ratis/server/storage/RaftStorageImpl.java Outdated
return new RaftStorageImpl.Builder();
}

public static class Builder {
Copy link
Contributor

@szetszwo szetszwo Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Builder should be moved to RaftStorage so that it will be in raft-server-api.

Copy link
Contributor Author

@codings-dan codings-dan Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

ratis-server/src/main/java/org/apache/ratis/server/storage/RaftStorageImpl.java Outdated
private StartupOption option;
private long storageFeeSpaceMin;

public Builder setDir(File dir) {
Copy link
Contributor

@szetszwo szetszwo Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename it to setDirectory.

Copy link
Contributor Author

@codings-dan codings-dan Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

ratis-server/src/main/java/org/apache/ratis/server/storage/RaftStorageImpl.java Outdated
return this;
}

public Builder setOption(StartupOption option) {
Copy link
Contributor

@szetszwo szetszwo Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should move StartupOption from RaftStorageImpl to RaftStorage so that it becomes a public API.

Also, let's add a new StartupOption.RECOVER instead of using null.

Copy link
Contributor Author

@codings-dan codings-dan Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

ratis-server/src/main/java/org/apache/ratis/server/storage/RaftStorageImpl.java Outdated
}

public RaftStorageImpl build() throws IOException {
return new RaftStorageImpl(dir, logCorruptionPolicy, option, storageFeeSpaceMin);
Copy link
Contributor

@szetszwo szetszwo Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After moved to raft-server-api, we have to use reflection to call the constructor. See RaftServer.Builder as an example.

Copy link
Contributor Author

@codings-dan codings-dan Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

ratis-server/src/main/java/org/apache/ratis/server/storage/RaftStorageImpl.java Outdated
return this;
}

public Builder setStorageFeeSpaceMin(long storageFeeSpaceMin) {
Copy link
Contributor

@szetszwo szetszwo Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call it setFreeSpaceMin. BTW, there is a TYPO: "Fee" should "Free".

Also, let's use SizeInBytes for the parameter.

Copy link
Contributor Author

@codings-dan codings-dan Jun 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@codings-dan
Copy link
Contributor Author

codings-dan commented Jun 13, 2022

@szetszwo PTAL again, thank you!

szetszwo
szetszwo approved these changes Jun 14, 2022
View reviewed changes
Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 the change looks good.

@szetszwo
Copy link
Contributor

szetszwo commented Jun 14, 2022

@codings-dan , The test failure seems related. Could you take a look?

BTW, let's remove the public RaftStorageImpl constructor below since it is no longer used.

  public RaftStorageImpl(File dir, CorruptionPolicy logCorruptionPolicy,
      long storageFeeSpaceMin) throws IOException {

Also, let's use SizeInBytes for storageFreeSpaceMin? Or, we may have two setStorageFreeSpaceMin methods.

    public Builder setStorageFreeSpaceMin(SizeInBytes storageFreeSpaceMin) {
      this.storageFreeSpaceMin = storageFreeSpaceMin.getSize();
      return this;
    }

    public Builder setStorageFreeSpaceMin(long storageFreeSpaceMin) {
      this.storageFreeSpaceMin = storageFreeSpaceMin;
      return this;
    }

@codings-dan
Copy link
Contributor Author

codings-dan commented Jun 16, 2022

The test failure seems related. Could you take a look

This is caused by improper handling of exceptions in reflection，I have fixed it.

let's remove the public RaftStorageImpl constructor below since it is no longer used.

I have remove the extra code.

let's use SizeInBytes for storageFreeSpaceMin

Done.

@codings-dan
Copy link
Contributor Author

codings-dan commented Jun 16, 2022

@szetszwo Thanks for help review the code, PTAL, thx!

@szetszwo
Copy link
Contributor

szetszwo commented Jun 16, 2022

@codings-dan , thanks for the update. TestRaftStorage failed. Please take a look.
https://github.com/apache/ratis/runs/6920545804?check_suite_focus=true#step:5:562

@codings-dan
Copy link
Contributor Author

codings-dan commented Jun 17, 2022 •
edited
Loading

TestRaftStorage failed.

Similar to the previous ut error, it was caused by improper handling of exceptions in reflection, I have fixed it

@codings-dan
Copy link
Contributor Author

codings-dan commented Jun 17, 2022

The ci test error seems have noting to do with this code change.

https://github.com/apache/ratis/runs/6930404226?check_suite_focus=true#step:5:611

@szetszwo
Copy link
Contributor

szetszwo commented Jun 17, 2022

Sure, let's restart the failed jobs.

@codings-dan
Copy link
Contributor Author

codings-dan commented Jun 17, 2022

https://github.com/apache/ratis/runs/6931402074?check_suite_focus=true#step:5:611

This unit test can run locally, see as below
image

@szetszwo
Copy link
Contributor

szetszwo commented Jun 17, 2022

#635 should be able to reduce the timeout problem. @codings-dan , could you review it?

@codings-dan
Copy link
Contributor Author

codings-dan commented Jun 17, 2022

#635 should be able to reduce the timeout problem. @codings-dan , could you review it?

Of course, happy to help review the code!

@szetszwo szetszwo merged commit 995dce9 into apache:master Jun 17, 2022
szetszwo pushed a commit that referenced this pull request Jun 18, 2022
@codings-dan @szetszwo
RATIS-1588. Add a builder of RaftStorageImpl (#652)
c9b2be6 
(cherry picked from commit 995dce9)
@codings-dan codings-dan deleted the 1587 branch June 20, 2022 01:51
SzyWilliam pushed a commit to SzyWilliam/ratis that referenced this pull request Jul 2, 2022
@codings-dan @SzyWilliam
RATIS-1588. Add a builder of RaftStorageImpl (apache#652)
a60a40b
symious pushed a commit to symious/ratis that referenced this pull request Mar 5, 2024
@codings-dan @symious
RATIS-1588. Add a builder of RaftStorageImpl (apache#652)
6217287
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@szetszwo szetszwo szetszwo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@codings-dan @szetszwo