Alluxio / alluxio

Alluxio, data orchestration for analytics and machine learning in the cloud
https://www.alluxio.io
Apache License 2.0
6.75k stars 2.92k forks source link

Avoid using Ratis internal API #17224

Open kaijchen opened 1 year ago

kaijchen commented 1 year ago

Summary Currently, Alluxio is using some internal API of Apache Ratis, causing compatibility issues during upgrades. For example: https://lists.apache.org/thread/2hzs61jzqjy0ktt6svv2ng6crh6t1f7q

Urgency It would be easier to upgrade Ratis in the future.

ssz1997 commented 1 year ago

@jenoudet @tcrain @yuzhu FYI

jenoudet commented 1 year ago

Thanks for raising the issue. Could you point us towards parts of the code where we use Ratis's internal API using link to our own codebase?

kaijchen commented 1 year ago

Thanks for raising the issue. Could you point us towards parts of the code where we use Ratis's internal API using link to our own codebase?

Here is all the imports of classes in ratis-server.

./core/server/common/src/main/java/alluxio/master/journal/raft/JournalStateMachine.java:53:import org.apache.ratis.statemachine.impl.BaseStateMachine;
./core/server/common/src/main/java/alluxio/master/journal/raft/JournalStateMachine.java:54:import org.apache.ratis.statemachine.impl.SimpleStateMachineStorage;
./core/server/common/src/main/java/alluxio/master/journal/raft/JournalStateMachine.java:55:import org.apache.ratis.statemachine.impl.SingleFileSnapshotInfo;
./core/server/common/src/main/java/alluxio/master/journal/raft/SnapshotReplicationManager.java:56:import org.apache.ratis.statemachine.impl.SimpleStateMachineStorage;
./core/server/common/src/main/java/alluxio/master/journal/raft/SnapshotReplicationManager.java:57:import org.apache.ratis.statemachine.impl.SingleFileSnapshotInfo;
./core/server/common/src/main/java/alluxio/master/journal/raft/SnapshotUploader.java:29:import org.apache.ratis.statemachine.impl.SimpleStateMachineStorage;
./core/server/common/src/main/java/alluxio/master/journal/raft/SnapshotDownloader.java:27:import org.apache.ratis.statemachine.impl.SimpleStateMachineStorage;
./core/server/common/src/main/java/alluxio/master/journal/raft/SnapshotDownloader.java:28:import org.apache.ratis.statemachine.impl.SingleFileSnapshotInfo;
./core/server/common/src/main/java/alluxio/master/journal/raft/RaftJournalUtils.java:15:import org.apache.ratis.statemachine.impl.SimpleStateMachineStorage;
./core/server/master/src/test/java/alluxio/master/journal/raft/RaftJournalTest.java:587:    Class<?> raftServerImpl = (Class.forName("org.apache.ratis.server.impl.RaftServerImpl"));
./core/server/master/src/test/java/alluxio/master/journal/raft/RaftJournalTest.java:597:    Class<?> raftServerImplClass = Class.forName("org.apache.ratis.server.impl.RaftServerImpl");
./core/server/master/src/test/java/alluxio/master/journal/raft/RaftJournalTest.java:602:    Class<?> serverStateClass = Class.forName("org.apache.ratis.server.impl.ServerState");
./core/server/master/src/test/java/alluxio/master/journal/raft/SnapshotReplicationManagerTest.java:47:import org.apache.ratis.server.storage.StorageImplUtils;
./core/server/master/src/test/java/alluxio/master/journal/raft/SnapshotReplicationManagerTest.java:48:import org.apache.ratis.statemachine.impl.SimpleStateMachineStorage;
./core/server/master/src/test/java/alluxio/master/journal/raft/SnapshotReplicationManagerTest.java:49:import org.apache.ratis.statemachine.impl.SingleFileSnapshotInfo;
./core/server/master/src/main/java/alluxio/master/journal/tool/RaftJournalDumper.java:23:import org.apache.ratis.server.raftlog.segmented.LogSegment;
./core/server/master/src/main/java/alluxio/master/journal/tool/RaftJournalDumper.java:24:import org.apache.ratis.server.raftlog.segmented.LogSegmentPath;
./core/server/master/src/main/java/alluxio/master/journal/tool/RaftJournalDumper.java:26:import org.apache.ratis.server.storage.StorageImplUtils;
./core/server/master/src/main/java/alluxio/master/journal/tool/RaftJournalDumper.java:27:import org.apache.ratis.statemachine.impl.SimpleStateMachineStorage;
./core/server/master/src/main/java/alluxio/master/journal/tool/RaftJournalDumper.java:28:import org.apache.ratis.statemachine.impl.SingleFileSnapshotInfo;
./tests/src/test/java/alluxio/server/ft/journal/raft/EmbeddedJournalIntegrationTestFaultTolerance.java:38:import org.apache.ratis.server.storage.StorageImplUtils;
./tests/src/test/java/alluxio/server/ft/journal/raft/EmbeddedJournalIntegrationTestFaultTolerance.java:39:import org.apache.ratis.statemachine.impl.SimpleStateMachineStorage;
./tests/src/test/java/alluxio/server/ft/journal/raft/EmbeddedJournalIntegrationTestFaultTolerance.java:40:import org.apache.ratis.statemachine.impl.SingleFileSnapshotInfo;
./tests/src/test/java/alluxio/client/cli/JournalToolTest.java:48:import org.apache.ratis.server.storage.StorageImplUtils;
./tests/src/test/java/alluxio/client/cli/JournalToolTest.java:49:import org.apache.ratis.statemachine.impl.SimpleStateMachineStorage;
./tests/src/test/java/alluxio/client/cli/JournalToolTest.java:50:import org.apache.ratis.statemachine.impl.SingleFileSnapshotInfo;

However, it's OK to reuse the implementation without using the internal API (so there might be false positives). For example:

// It's ok to reuse the implementation
StateMachineStorage storage = new SimpleStateMachineStorage();

// But it's not good to use the internal API
SimpleStateMachineStorage storage = new SimpleStateMachineStorage();
kaijchen commented 1 year ago

This issue is partially fixed by #16998

github-actions[bot] commented 3 weeks ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions.