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

[IOTDB-915]Add raft log persist mechanism and use persist log to catch up #1865

Conversation

neuyilan
Copy link
Member

No description provided.

@neuyilan neuyilan marked this pull request as draft October 26, 2020 11:34
@HTHou HTHou added the Module - Cluster PRs for the cluster module label Oct 26, 2020
@neuyilan neuyilan force-pushed the apache_cluster_new_1023_raft_log_catch_up branch from f075e52 to 7caca25 Compare October 27, 2020 12:28
@neuyilan neuyilan marked this pull request as ready for review October 28, 2020 01:42
@neuyilan neuyilan marked this pull request as draft October 28, 2020 01:55
@neuyilan neuyilan marked this pull request as ready for review October 28, 2020 02:29
@neuyilan neuyilan marked this pull request as draft October 28, 2020 02:48
@neuyilan neuyilan changed the title [IOTDB-915]Add raft log mechanism and use persist log to catch up [IOTDB-915]Add raft log persist mechanism and use persist log to catch up Oct 29, 2020
@neuyilan neuyilan marked this pull request as ready for review October 29, 2020 08:21
Copy link
Contributor

@OneSizeFitsQuorum OneSizeFitsQuorum left a comment

Choose a reason for hiding this comment

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

Great Work!

@@ -292,15 +292,29 @@ public long getLastLogIndex() {
public long getTerm(long index) throws EntryCompactedException {
long dummyIndex = getFirstIndex() - 1;
if (index < dummyIndex) {
// search in disk
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought these code should be in the function maybeTerm in committedEntryManager, which should manager all persisted entries, it will throw a EntryCompactedException or get log from disk if isEnableRaftLogPersistence is enabled.
BTW, I'm a little confused about whether we should get log from disk in function getTerm,If so, then maybe we should change isEnableUsePersistLogOnDiskToCatchUp's name

Copy link
Member Author

Choose a reason for hiding this comment

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

if we manage all the persisted entries(committed or persisted) on the committedEntryManager, it's better to move the code in committedEntryManager. but now, the committedEntryManager only manager the commit log in memory. and the LogManager managers the UnCommittedEntryManager CommittedEntryManager and StableEntryManager, I think CommittedEntryManager is as logs in memory, StableEntryManager is as logs in the disk, so I think it's ok to leave the code here, the log manager knows where to get the logs

besides, I argue with you that, when get term, we should not care the isEnableUsePersistLogOnDiskToCatchUp parpermeter, if isEnableRaftLogPersistence is enabled is ok

Copy link
Contributor

@OneSizeFitsQuorum OneSizeFitsQuorum Nov 1, 2020

Choose a reason for hiding this comment

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

OK.As the logmanager has been given more functionality, I will try to merge UnCommittedEntryManager and CommittedEntryManager to a MemoryLogmanager to avoid redundant entry shift laterly.
I'm OK with here~

.allocate(ClusterDescriptor.getInstance().getConfig().getRaftLogBufferSize());
private ByteBuffer logIndexBuffer = ByteBuffer
Copy link
Contributor

@OneSizeFitsQuorum OneSizeFitsQuorum Oct 30, 2020

Choose a reason for hiding this comment

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

As you have used logIndexOffsetList to record offset in putLog, I doubt whether it's necessary to maintain this buffer in putLog.Maybe You can generate index buffer in flushLogBuffer .

Copy link
Member Author

Choose a reason for hiding this comment

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

I designed it on purpose. If you set a temporary buffer, GC will occur. I think it is better to set a buffer that is not GC?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK~

* the offset of the log's index, for example, the first value is the offset of index
* ${firstLogIndex}, the second value is the offset of index ${firstLogIndex+1}
*/
private List<Long> logIndexOffsetList;
Copy link
Contributor

Choose a reason for hiding this comment

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

As currently the default maxSize of each data file is 1G,it's seems a Integer is able to record the offset.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, but the parameter of the max size of the log data file is left to the user to set, so if the user config larger than an integer, it would occur errors, or we could limit the range of user configuration for the maxSize parameter.

Copy link
Contributor

@OneSizeFitsQuorum OneSizeFitsQuorum Oct 31, 2020

Choose a reason for hiding this comment

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

OK, Just let it go~Long is OK for me ~

Copy link
Contributor

@jt2594838 jt2594838 left a comment

Choose a reason for hiding this comment

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

Great, only minor problems.

minAvailableVersion = getFileVersion(logFile);
serializeMeta(meta);
@Override
public List<Log> getLogs(long startIndex, long endIndex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should enforce a limit on this method to avoid out ot memory when the range is too long.

Copy link
Member Author

Choose a reason for hiding this comment

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

I add one parameter to limit the max number of logs per fetch

@neuyilan
Copy link
Member Author

neuyilan commented Nov 1, 2020

#1865 (comment)

the log whose index large than the applied index is all thought has been applied, the wal may make sure when re start iotdb to redo the logs.

@jt2594838
Copy link
Contributor

It seems we got compilation error.

@jt2594838 jt2594838 merged commit 4715ad5 into apache:cluster_new Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module - Cluster PRs for the cluster module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants