Skip to content

Remove versionInfo in Tsfile and get rid of versions in memtable#2445

Merged
HTHou merged 27 commits intoapache:masterfrom
wshao08:mod_fix
Jan 15, 2021
Merged

Remove versionInfo in Tsfile and get rid of versions in memtable#2445
HTHou merged 27 commits intoapache:masterfrom
wshao08:mod_fix

Conversation

@wshao08
Copy link
Copy Markdown
Contributor

@wshao08 wshao08 commented Jan 7, 2021

IoTDB uses an incremental field "version" to keep track of the updating order of multiple Memtables, and this version is also persisted into TsFiles.
This PR targets to remove the "version" field in Memtables and avoid serializing/deserializing it into TsFiles.

Design docs:
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=173082421

@wshao08 wshao08 marked this pull request as draft January 7, 2021 09:55
@wshao08 wshao08 marked this pull request as ready for review January 11, 2021 06:19
Copy link
Copy Markdown
Contributor

@SilverNarcissus SilverNarcissus left a comment

Choose a reason for hiding this comment

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

Fantastic work! I think we need some test for a special situation. There are tests for deletion in memtable and tsfile but no test for deletion in a flushing memtable. Please add some tests for it~

ChunkMetadata metaData = chunkMetaData.get(metaIndex);
for (Modification modification : modifications) {
if (modification.getVersionNum() > metaData.getVersion()) {
if (modification.getFileOffset() > metaData.getOffsetOfChunkHeader()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can modification.getFileOffset() == metaData.getOffsetOfChunkHeader() ? What should we do in this condition

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. metaData.getOffsetOfChunkHeader() marks the byte offset of this chunk header.
  2. modification.getFileOffset() returns the offset of any chunk in the file after the chunk has been flushed.
    So I consider it could be guaranteed that if any chunk is successfully flushed, offset in (2) > (1).
    Thats why I didn't add the equality condition.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reasonable! So just add some comment here and add test which I said above.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can also check sonar code smell. There is an unused parameter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'll add some more test scenarios.

Copy link
Copy Markdown
Contributor

@HTHou HTHou left a comment

Choose a reason for hiding this comment

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

I checked the code related to Flushing and TsFile module, just some small log issues. Please fix them.

Comment thread server/src/main/java/org/apache/iotdb/db/engine/flush/MemTableFlushTask.java Outdated
Comment thread server/src/main/java/org/apache/iotdb/db/engine/flush/MemTableFlushTask.java Outdated
Comment thread server/src/main/java/org/apache/iotdb/db/engine/flush/MemTableFlushTask.java Outdated
Comment thread server/src/main/java/org/apache/iotdb/db/engine/flush/MemTableFlushTask.java Outdated
Comment thread server/src/main/java/org/apache/iotdb/db/engine/flush/MemTableFlushTask.java Outdated
Comment thread server/src/main/java/org/apache/iotdb/db/engine/flush/MemTableFlushTask.java Outdated
Comment thread server/src/main/java/org/apache/iotdb/db/engine/flush/MemTableFlushTask.java Outdated
Comment thread server/src/main/java/org/apache/iotdb/db/engine/flush/MemTableFlushTask.java Outdated
Comment thread server/src/main/java/org/apache/iotdb/db/engine/flush/MemTableFlushTask.java Outdated
@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@HTHou HTHou merged commit ede19b5 into apache:master Jan 15, 2021
@wshao08 wshao08 deleted the mod_fix branch January 29, 2021 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants