feat(wal): optimize the start-up and shutdown of WAL #1625

CLFutureX · 2024-07-23T08:05:57Z

Motivation ：
As a core component of AutoMQ that is closely related to memory resources, the initialization and termination processes of WAL must ensure a high degree of robustness. However, there are currently some flaws in these processes that fail to achieve a complete operational loop. This means that during the startup and shutdown processes, all relevant resources may not be properly handled, which can affect the stability and reliability of the system. It is necessary to further optimize these processes to ensure they can manage memory resources seamlessly from start to finish, avoiding potential resource leaks or other issues.

CLFutureX · 2024-07-26T07:50:48Z

@superhx @Chillax-0v0 hey, PTAL

Chillax-0v0

Hi @CLFutureX,

I apologize for the delay in reviewing this PR. Thank you for your patience and for the effort you've put into this work.

I've gone through the changes and have a few suggestions that I believe will improve the implementation. Could you please consider these modifications?

Thank you again for your contribution!

Chillax-0v0 · 2024-07-30T06:18:59Z

s3stream/src/main/java/com/automq/stream/s3/wal/impl/block/BlockWALService.java

+        switch (WAL_STATE.get(this)) {
+            case WAL_STATE_INIT:
+                if (WAL_STATE.compareAndSet(this, WAL_STATE_INIT, WAL_STATE_STARTED)) {
+                    boolean success = false;
+                    try {
+                        doStart();
+                        success = true;
+                    } finally {
+                        if (!success) {
+                            WAL_STATE.compareAndSet(this, WAL_STATE_STARTED, WAL_STATE_INIT);
+                            LOGGER.warn("block WAL service started fail");
+                        }
+                    }
+                }
+                break;
+            case WAL_STATE_STARTED:
+                LOGGER.warn("block WAL service already started");
+                break;


Currently, the "started" state is primarily used to check whether the metadata required for WAL operation (e.g., WAL Header) is ready (see callers of checkStarted), in order to avoid data corruption caused by premature writes.

Therefore, the state can only be set to "started" after the doStart method executes successfully.

Adding an intermediate "starting" state might resolve the issue.

Chillax-0v0 · 2024-07-30T06:38:24Z

s3stream/src/main/java/com/automq/stream/s3/wal/impl/block/BlockWALService.java

+    private static final int WAL_STATE_STARTED = 2;
+    private static final int WAL_STATE_SHUTING_DOWN = 3;
+    private static final int WAL_STATE_SHUTDOWN = 4;
+    private static final AtomicIntegerFieldUpdater<BlockWALService> WAL_STATE = AtomicIntegerFieldUpdater.newUpdater(BlockWALService.class, "state");


I think there is no need to use AtomicIntegerFieldUpdater here; an AtomicReference<State> should suffice. Here are a few reasons:

In our scenario, there won't be many WAL instances simultaneously (in fact, in most cases, there is only one). So a global static updater is unnecessary.

Compared to AtomicIntegerFieldUpdater, AtomicReference can provide better code readability and maintainability.

I conducted a simple benchmark, and there is no significant performance difference between them (in fact, for the most frequently called get method in our use case, AtomicReference performs slightly better).

Chillax-0v0 · 2024-07-30T06:41:05Z

s3stream/src/main/java/com/automq/stream/s3/wal/impl/block/BlockWALService.java

+    private static final int WAL_STATE_INIT = 1;
+    private static final int WAL_STATE_STARTED = 2;
+    private static final int WAL_STATE_SHUTING_DOWN = 3;
+    private static final int WAL_STATE_SHUTDOWN = 4;


If we use AtomicReference to maintain the state of WAL (as I mentioned above), then these int constants can also be replaced with a separate enum class.

# Conflicts: # s3stream/src/main/java/com/automq/stream/s3/wal/impl/block/BlockWALService.java

CLFutureX · 2024-07-31T07:07:03Z

Hi @CLFutureX,

I apologize for the delay in reviewing this PR. Thank you for your patience and for the effort you've put into this work.

I've gone through the changes and have a few suggestions that I believe will improve the implementation. Could you please consider these modifications?

Thank you again for your contribution!

Thank you for your reply, I am very willing to make the modifications according to your suggestions.

Hi @CLFutureX,

I apologize for the delay in reviewing this PR. Thank you for your patience and for the effort you've put into this work.

I've gone through the changes and have a few suggestions that I believe will improve the implementation. Could you please consider these modifications?

Thank you again for your contribution!

Thank you for your reply. I am happy to modify according to your suggestions:

Firstly, I plan to create a new WalState enum and the corresponding AtomicReference object as advised

enum WalState {
INIT(1),
STARTING(2),
STARTED(3),
SHUTTING_DOWN(4),
SHUTDOWN(5);
private final int state;
WalState(int state) {
this.state = state;
}
}

However, during the modification process, I found that the state branch judgment in the start() method is not very
friendly due to the syntax of switch-case, which leads to the need to rewrite the conditional branches as if-else.
Therefore, I chose another method based on final constants plus AtomicInteger.

Secondly, a starting state has been added to the state machine. The state is only changed to started after a successful startup.

Signed-off-by: Ning Yu <[email protected]>

Chillax-0v0 · 2024-08-21T10:00:10Z

@CLFutureX Hello, I have submitted a PR to your branch CLFutureX#1, which converts WalState into an enum class. Please take a look.

refactor(s3stream/wal): use an enum class to identify the WAL state

CLFutureX · 2024-08-22T01:57:06Z

@CLFutureX Hello, I have submitted a PR to your branch CLFutureX#1, which converts WalState into an enum class. Please take a look.

LGTM

github-actions · 2024-11-21T03:37:42Z

This PR is being marked as stale since it has not had any activity in 90 days. If you would like to keep this PR alive, please ask a committer for review. If the PR has merge conflicts, please update it with the latest from trunk (or appropriate release branch)

If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed.

CLFutureX added 2 commits July 18, 2024 17:30

feat(wal): wal start optimization

e5c5a24

feat(wal): wal start optimization

bf58d41

CLFutureX requested review from superhx, SCNieh, ShadowySpirits and Chillax-0v0 as code owners July 23, 2024 08:05

CLFutureX added 2 commits July 24, 2024 11:41

feat(wal): wal start optimization

0323232

feat(wal): wal start and shutdown optimization

b90d210

CLFutureX changed the title ~~feat(wal): wal start optimization~~ feat(wal): Optimize the start-up and shutdown of WAL Jul 24, 2024

Chillax-0v0 reviewed Jul 30, 2024

View reviewed changes

Chillax-0v0 changed the title ~~feat(wal): Optimize the start-up and shutdown of WAL~~ feat(wal): optimize the start-up and shutdown of WAL Jul 30, 2024

CLFutureX added 5 commits July 31, 2024 13:37

feat(wal): wal start and shutdown optimization

ce25681

feat(wal): wal start and shutdown optimization

84c4424

feat(wal): wal start and shutdown optimization

6e50d72

Merge branch 'main' into PiteChen_feat_wal_start

ce01b42

# Conflicts: # s3stream/src/main/java/com/automq/stream/s3/wal/impl/block/BlockWALService.java

feat(wal): wal start and shutdown optimization

62982cf

CLFutureX requested a review from Chillax-0v0 July 31, 2024 07:44

feat(s3stream): wal start and shutdown optimization

d07a683

CLFutureX mentioned this pull request Aug 20, 2024

feat(wal): failoverTest optimization #1871

Merged

refactor(s3stream/wal): use an enum class to identify the WAL state

c8101dc

Signed-off-by: Ning Yu <[email protected]>

Merge pull request #1 from Chillax-0v0/Chillax_refactor_wal_state

b43cd52

refactor(s3stream/wal): use an enum class to identify the WAL state

github-actions bot added the Stale label Nov 21, 2024

daniel-y requested a review from Gezi-lzq as a code owner December 30, 2024 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(wal): optimize the start-up and shutdown of WAL #1625

feat(wal): optimize the start-up and shutdown of WAL #1625

CLFutureX commented Jul 23, 2024 •

edited

Loading

CLFutureX commented Jul 26, 2024

Chillax-0v0 left a comment

Chillax-0v0 Jul 30, 2024 •

edited

Loading

Chillax-0v0 Jul 30, 2024

Chillax-0v0 Jul 30, 2024 •

edited

Loading

CLFutureX commented Jul 31, 2024

Chillax-0v0 commented Aug 21, 2024

CLFutureX commented Aug 22, 2024

github-actions bot commented Nov 21, 2024

feat(wal): optimize the start-up and shutdown of WAL #1625

Are you sure you want to change the base?

feat(wal): optimize the start-up and shutdown of WAL #1625

Conversation

CLFutureX commented Jul 23, 2024 • edited Loading

CLFutureX commented Jul 26, 2024

Chillax-0v0 left a comment

Choose a reason for hiding this comment

Chillax-0v0 Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Chillax-0v0 Jul 30, 2024

Choose a reason for hiding this comment

Chillax-0v0 Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

CLFutureX commented Jul 31, 2024

Chillax-0v0 commented Aug 21, 2024

CLFutureX commented Aug 22, 2024

github-actions bot commented Nov 21, 2024

CLFutureX commented Jul 23, 2024 •

edited

Loading

Chillax-0v0 Jul 30, 2024 •

edited

Loading

Chillax-0v0 Jul 30, 2024 •

edited

Loading