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-351] Serialize raft log #958

Merged

Conversation

SilverNarcissus
Copy link
Contributor

Serialize raft log in disk

… into IOTDB-351-serialize-raft-log

# Conflicts:
#	cluster/src/test/java/org/apache/iotdb/cluster/common/TestUtils.java
… into IOTDB-351-serialize-raft-log

# Conflicts:
#	cluster/src/main/java/org/apache/iotdb/cluster/log/manage/FilePartitionedSnapshotLogManager.java
#	cluster/src/main/java/org/apache/iotdb/cluster/log/manage/MemoryLogManager.java
#	cluster/src/main/java/org/apache/iotdb/cluster/log/manage/MetaSingleSnapshotLogManager.java
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.

The robustness and efficiency of merely using one log file and copying the undeleted part to a new file are doubted, because the system may go down between changing the file and modifying metadata.

It is advised that when the size of the logs reaches the removal threshold, open a new file for future logs.

@SilverNarcissus
Copy link
Contributor Author

I will design a list of log files for robustness and efficiency in these days~

@jt2594838
Copy link
Contributor

I will design a list of log files for robustness and efficiency in these days~

Looking forward to it. Please contact me with your design doc when you finish it.

@OneSizeFitsQuorum
Copy link
Contributor

OneSizeFitsQuorum commented Apr 2, 2020

I will design a list of log files for robustness and efficiency in these days~

If you want to design a list of log files for log persistence, these code may be helpful for you~

}


public void truncateLog(int count) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The truncate function can be achieved at this level by taking advantage of the fact that the indexes of the raft logs must be contiguous, rather than being invoked from above. Truncate should be a function of append function rather than an interface.I'm sorry that I misdescribed it in a way that led to this implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, truncate and append can be easily achieved by calling truncate method and append method

Copy link
Contributor

@OneSizeFitsQuorum OneSizeFitsQuorum Apr 7, 2020

Choose a reason for hiding this comment

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

Ok~Furthermore, the upper layer doesn't call the truncate interface with the current implementation. If the memoryLogManager is going to truncate wrong logs at append function then the diskLogManager also needs to have the corresponding implementation, so the diskLogManager’s append function should also need some changes, such as calling the truncate function. Of course, you won't need to implement this for append if it's embedded in new design in the future.Just for reminding ~

public void setLastLogId(long lastLogId) {
super.setLastLogId(lastLogId);
// save meta
serializeMeta();
Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, for the implementation of memoryLogManager, the lastLogTerm does not need to be maintained this way manually, because you can simply take the index of the last log in bufffer. If memoryLogManager could make this small change, here would be no need to serialize the meta data.This may require further discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but this class may be discard when integrating with your code. You can decide whether to hold this

Copy link
Contributor

Choose a reason for hiding this comment

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

OK~


private long commitLogIndex = -1;
private long lastLogId = -1;
private long lastLogTerm = -1;
Copy link
Contributor

@OneSizeFitsQuorum OneSizeFitsQuorum Apr 5, 2020

Choose a reason for hiding this comment

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

Actually, I don't find out the meaning of persisting lastLogId and lastLogTerm because both of them are available from the lastLog after recoverLog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but considering the nature of the disk (write two more long will cause almost no impact to the performance) and the abstract level of SyncLogDequeSerializer. I just see all the variable of memoryLogManager as meta

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine~

}

/**
* log in disk is size of log | log buffer meta in disk is firstLogPosition | size of log meta |
Copy link
Contributor

@OneSizeFitsQuorum OneSizeFitsQuorum Apr 5, 2020

Choose a reason for hiding this comment

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

Sorry,I don't figure out the comment here. Is this the format of file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is the format of the log file. I will make it clearly later

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks~

… into IOTDB-351-serialize-raft-log

# Conflicts:
#	cluster/src/main/java/org/apache/iotdb/cluster/log/manage/MemoryLogManager.java
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.

LGTM

… into IOTDB-351-serialize-raft-log

# Conflicts:
#	cluster/src/main/java/org/apache/iotdb/cluster/config/ClusterDescriptor.java
import org.apache.iotdb.cluster.log.Log;

public interface LogDequeSerializer {

Copy link
Contributor

Choose a reason for hiding this comment

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

I want to add a new function “void append(List entries)” to support batch append so that it can be easily embedded with new design.You can do it without thinking about efficiency, and you can rewrite it after I've merged it. Of course, it would be better to focus on efficiency in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to add another function "void removeCompactedEntries(long index)" which means the persisted logs which index prior to index is able to be deleted.Of course, you can delete it lazily.But as we are not clear when to delete persisted raft logs so far,so you can implement it later.Just for reminding~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks~, I will add “void append(List entries)” and removeCompactedEntries can be achieved by remove function. In LogDequeSerializer, we don't know index but only the location of the logs. So it's memory log manager's responsibility to use index to find how many log we should remove.


// do actual deletion
if (removedLogSize > maxRemovedLogSize) {
openNewLogFile();
Copy link
Contributor

Choose a reason for hiding this comment

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

After openNewLogFile()removedLogSize > maxRemovedLogSize is still true. Consider the case, assuming the size of each log is 1:
initially: removedLogSize=50, maxRemovedLogSize=50, currentLogFileSize=100
If I call removeFirst(1) for ten times, each time removedLogSize > maxRemovedLogSize will be true and a new file will be open, even if the last log file may be empty.

My suggestion is: do not open new files during removals but during appending.
For example, each time the size of the current file >= maxRemovedLogSize after appending, open a new file, so the log files will have sizes around maxRemovedLogSize. And each time when removedLogSize > firstLogFile.length() after removal, you can just remove the first one or more files and set removedLogSize -= removedFile.length().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That won't happen because in openNewLogFile, we firstly call actuallyDeleteFile, in this function we call adjustNextThreshold, maxRemovedLogSize will become the first log file length.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that the log file becomes very small?
For example:
When you close the 1st file, the length of the 1st file is 100, so you set maxRemovedLogSize to 100. But when removedLogSize reaches 100, the length of the 2nd file may only be 10, so you will set maxRemovedLogSize to 10.

@jt2594838 jt2594838 added the Module - Cluster PRs for the cluster module label Apr 13, 2020
@jt2594838 jt2594838 merged commit 3215776 into apache:cluster_new Apr 14, 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.

None yet

3 participants