Skip to content

[IOTDB-165][TsFile] Delete a current version and add a number version and an exception.#389

Closed
Genius-pig wants to merge 0 commit intoapache:masterfrom
Genius-pig:master
Closed

[IOTDB-165][TsFile] Delete a current version and add a number version and an exception.#389
Genius-pig wants to merge 0 commit intoapache:masterfrom
Genius-pig:master

Conversation

@Genius-pig
Copy link
Contributor

According to this issue, I implement 1, 2, delete a current version which I think is useless, and I will add an exception for TsFile reader in a few days. But discussion of the compatibility between iotdb and TsFile is still continuing.

  1. Change current TsFile magic number from 12 bytes to: 6 bytes magic string ("TsFile") + 6 bytes version number ({"000001", "000002", ""000003"}) ("v0.8.0" is the first version (we treat it as "000000")).We update the TsFile version unless the format is incompatible.
  1. The tail of a TsFile only has "TsFile" magic string, without the version.

@Genius-pig Genius-pig changed the title [IOTDB-165][TsFile] Delete a current version and add a number version [IOTDB-165][TsFile] Delete a current version and add a number version and an exception. Sep 14, 2019
@Genius-pig
Copy link
Contributor Author

Genius-pig commented Sep 14, 2019

It failed in windows platform, there was some error information, but I don't know why.

[ERROR] Tests run: 2, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 0.014 s <<< FAILURE! - in org.apache.iotdb.tsfile.read.TsFileSequenceReaderTest
[ERROR] testReadTsFileSequently(org.apache.iotdb.tsfile.read.TsFileSequenceReaderTest)  Time elapsed: 0.014 s  <<< ERROR!
java.io.FileNotFoundException: target\testTsFile.tsfile (Access is denied)
	at org.apache.iotdb.tsfile.read.TsFileSequenceReaderTest.before(TsFileSequenceReaderTest.java:54)
[ERROR] testReadTsFileSequently(org.apache.iotdb.tsfile.read.TsFileSequenceReaderTest)  Time elapsed: 0.014 s  <<< ERROR!
java.lang.NullPointerException
	at org.apache.iotdb.tsfile.read.TsFileSequenceReaderTest.after(TsFileSequenceReaderTest.java:61)

@Genius-pig Genius-pig force-pushed the master branch 2 times, most recently from 04aef07 to 805fa9b Compare September 15, 2019 08:15
@Genius-pig Genius-pig force-pushed the master branch 3 times, most recently from 9a7a3a9 to e623906 Compare September 18, 2019 12:31
Copy link
Member

@jixuan1989 jixuan1989 left a comment

Choose a reason for hiding this comment

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

One question that want to discuss: do we need the version number at the tail of the file?

@Genius-pig Genius-pig force-pushed the master branch 4 times, most recently from c3d148f to e060369 Compare September 18, 2019 15:56
@Genius-pig
Copy link
Contributor Author

@jixuan1989 I think don't need it, because when reading the head magic, you have already known the compatibility. The tail magic is used for an end sign. BTW, it's just my opinions.

}
}

fileMetaData.currentVersion = ReadWriteIOUtils.readInt(inputStream);
Copy link
Member

Choose a reason for hiding this comment

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

Remind to maintain the format-changelist.md

return tsFileInput.size() >= TSFileConfig.MAGIC_STRING.length() * 2 && readTailMagic()
.equals(readHeadMagic());
return tsFileInput.size() >= TSFileConfig.MAGIC_STRING.length() * 2 + TSFileConfig.VERSION_NUMBER.length() &&
(readTailMagic() + TSFileConfig.VERSION_NUMBER).equals(readHeadMagic());
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, I think we can use two methods: readHeadMagic() which return "TsFile" and readVersionNumber(). We do not need to do operations like "String + String" (e.g., readTailMagic() + TSFileConfig.VERSION_NUMBER), which waste memory..

@@ -213,15 +214,26 @@ public String readHeadMagic() throws IOException {
* to the end of the magic head string.
*/
public String readHeadMagic(boolean movePosition) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

throws NotCompatibleException, IOException

/**
* this function checks compatibility of TsFile.
*/
public void checkHeadMagic(String headMagic) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

throws NotCompatibleException

* this function checks compatibility of TsFile.
*/
public void checkHeadMagic(String headMagic) throws IOException {
if(!headMagic.equals(TSFileConfig.MAGIC_STRING + TSFileConfig.VERSION_NUMBER)) {
Copy link
Member

Choose a reason for hiding this comment

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

same question, do not need to generate a new String by using +.

expressionOptimizer.optimize(expression, selectedSeries).toString());

} catch (QueryFilterOptimizationException e) {
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

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

Can you replace this e.print with logger.error()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use a Assert.fail() is better?

Copy link
Member

@jixuan1989 jixuan1989 left a comment

Choose a reason for hiding this comment

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

Remind:

  1. do not use string + string unless you have to do that.
  2. record all the changes if the file format is changed.

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.

2 participants