Skip to content
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.

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

RATIS-2093. Decouple metadata and configuration entries from appendEntries buffer for stateMachineCache' #1096

Merged
merged 2 commits into from
May 24, 2024

Conversation

duongkame
Copy link
Contributor

@duongkame duongkame commented May 16, 2024

What changes were proposed in this pull request?

See RATIS-2093.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/RATIS-2093

How was this patch tested?

@duongkame duongkame changed the title RATIS-2093. Decouple metadata and configuration entries from appendEn… RATIS-2093. Decouple metadata and configuration entries from appendEntries buffer for stateMachineCache' May 16, 2024
@duongkame
Copy link
Contributor Author

@szetszwo can you have a look?

@duongkame duongkame marked this pull request as ready for review May 24, 2024 18:34
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.

@duongkame , thanks for working on this! Please see the comments inlined.

ReferenceCountedObject.wrap(removedStateMachineData));
} else {
cache.appendEntry(LogSegment.Op.WRITE_CACHE_WITH_STATE_MACHINE_CACHE,
ReferenceCountedObject.wrap(LogProtoUtils.copy(entry)));
Copy link
Contributor

Choose a reason for hiding this comment

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

It should copy removedStateMachineData instead.

// The stateMachineData will be cached inside the StateMachine itself.
cache.appendEntry(LogSegment.Op.WRITE_CACHE_WITH_STATE_MACHINE_CACHE,
ReferenceCountedObject.wrap(removedStateMachineData));
if ((removedStateMachineData != entry)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are double parenthesis, i.e. ((...))

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 szetszwo merged commit e002587 into apache:master May 24, 2024
12 checks passed
@duongkame
Copy link
Contributor Author

Thanks for merging it @szetszwo . Sorry, I have to ask for another favor, can release a snapshot version after this commit?

@szetszwo
Copy link
Contributor

@duongkame , deployed 3.1.0-640749b-SNAPSHOT.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants