[IOTDB-868] fix comma bug for mlog by mychaow · Pull Request #1721 · apache/iotdb

mychaow · 2020-09-10T09:01:12Z

After we import the double-quoted path, users may have any characters in the path that includes ",".

If we still record the mlog using csv format, we could not operate correctly to paths like

root.changde.e34c49.struct@VALUE%PRESENT_ANGLE,_CUTTING_HEAD]

The mlog needs to be encoded.

https://issues.apache.org/jira/browse/IOTDB-868

samperson1997

Hi Chao, thanks for your contribution! This is a very vital fix. From my first view, I have a few questions to discuss. Could you help and answer them?

server/src/main/java/org/apache/iotdb/db/writelog/io/LogWriter.java

server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfigCheck.java

server/src/main/java/org/apache/iotdb/db/writelog/io/LogWriter.java

server/src/test/java/org/apache/iotdb/db/integration/IoTDBCreateSnapshotIT.java

server/src/main/java/org/apache/iotdb/db/sync/receiver/transfer/SyncServiceImpl.java

server/src/main/java/org/apache/iotdb/db/qp/physical/sys/StorageGroupMNodePlan.java

server/src/main/java/org/apache/iotdb/db/metadata/logfile/MLogWriter.java

docs/SystemDesign/SchemaManager/SchemaManager.md

docs/zh/SystemDesign/SchemaManager/SchemaManager.md

server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfigCheck.java

server/src/main/java/org/apache/iotdb/db/metadata/MTree.java

server/src/main/java/org/apache/iotdb/db/metadata/MManager.java

server/src/main/java/org/apache/iotdb/db/metadata/logfile/MLogWriter.java

qiaojialin

Good work! It really a lot effort. Finding a way to replace MNodePlan by MNode is better.

server/src/assembly/resources/conf/iotdb-engine.properties

server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfig.java

server/src/main/java/org/apache/iotdb/db/metadata/logfile/MLogWriter.java

qiaojialin · 2020-09-14T13:19:44Z

server/src/main/java/org/apache/iotdb/db/metadata/logfile/OldMLogReader.java

+import java.io.IOException;
+
+
+public class OldMLogReader implements AutoCloseable {


rename to MLogTXTReader or add a javadoc : for reading mlog.txt

qiaojialin · 2020-09-14T13:28:35Z

server/src/main/java/org/apache/iotdb/db/metadata/logfile/MLogWriter.java

+    try {
+      plan.serialize(mlogBuffer);
+    } catch (BufferOverflowException e) {
+      logger.error("MLog {} BufferOverflow !", plan.getOperatorType(), e);


This is not an error case, change this to debug level is better. The case is: Allocate 16MB for buffer and put log into this buffer one by one, In the end, it always trigger the BufferOverflowException, just reseting the buffer is ok.

However, if one log exceeds 16M, this will throw an exception.

qiaojialin · 2020-09-14T13:31:14Z

server/src/main/java/org/apache/iotdb/db/metadata/logfile/MLogWriter.java

+  private File logFile;
+  private LogWriter logWriter;
+  private int logNum;
+  private ByteBuffer mlogBuffer = ByteBuffer.allocate(


If using a buffer like WAL, we also need a thread to sync the buffer periodically like the forceTask in MultiFileLogNodeManager. Otherwise, the last logs in the buffer will never be persisted.

One option is to sync the mlog one by one.

yes, I think maybe we need to call the forceTask periodically. Because FileChannel.force(true) is more costful than bufferedWriter.flush, this will result in performance degradation of metadata operation.

qiaojialin · 2020-09-14T13:52:25Z

server/src/main/java/org/apache/iotdb/db/metadata/MTree.java

+          if (plan instanceof StorageGroupMNodePlan) {
+            node = StorageGroupMNode.deserializeFrom((StorageGroupMNodePlan) plan);
+            childrenSize = ((StorageGroupMNodePlan) plan).getChildSize();
+          } else if (plan instanceof MeasurementMNodePlan) {
+            node = MeasurementMNode.deserializeFrom((MeasurementMNodePlan) plan);
+            childrenSize = ((MeasurementMNodePlan) plan).getChildSize();
+          } else if (plan instanceof MNodePlan) {
+            node = new MNode(null, ((MNodePlan) plan).getName());
+            childrenSize = ((MNodePlan) plan).getChildSize();
+          }


The XXMNodePlan is almost the same as the XXMNode, the structure is not a problem. But creating or recovering a snapshot may be slower in this way. Better to test the performance. If the performance does not decrease a lot, this is acceptable. We could test 10M timeseries (10000 device * 1000 measurement)

ok, I will do some tests.

I have done some tests, 1M timeseries, the cpu cost it almost same.

qiaojialin · 2020-09-14T13:56:37Z

server/src/main/java/org/apache/iotdb/db/metadata/MTree.java

-        new FileWriter(SystemFileFactory.INSTANCE.getFile(snapshotPath)))) {
-      root.serializeTo(bw);
+    try (MLogWriter mLogWriter = new MLogWriter(snapshotPath)) {
+      root.serializeTo(mLogWriter);


Pay attention to force mLogWriter before close it.

HTHou · 2020-10-10T09:38:13Z

There are some conflicts now, please take a look and fix them.

mychaow · 2020-10-12T07:19:57Z

There are some conflicts now, please take a look and fix them.

fix

samperson1997

Hi @mychaow , the parser is quite convenient and I really appreciate your work. Here I have some questions about the details, please have a look ; )

server/src/main/java/org/apache/iotdb/db/tools/mlog/MLogParser.java

server/src/main/java/org/apache/iotdb/db/qp/physical/sys/StorageGroupMNodePlan.java

server/src/main/java/org/apache/iotdb/db/metadata/logfile/MLogTxtReader.java

server/src/main/java/org/apache/iotdb/db/metadata/MTree.java

server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfigCheck.java

docs/UserGuide/System Tools/MLogParser Tool.md

docs/zh/UserGuide/System Tools/MLogParser Tool.md

server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfigCheck.java

server/src/main/java/org/apache/iotdb/db/metadata/logfile/MLogWriter.java

server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfigCheck.java

server/src/main/java/org/apache/iotdb/db/qp/logical/Operator.java

qiaojialin · 2020-12-08T04:48:30Z

server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfigCheck.java

-    if (!properties.containsKey(IOTDB_VERSION_STRING)) {
-      logger.error("DO NOT UPGRADE IoTDB from v0.9 or lower version to v0.11!"
-          + " Please upgrade to v0.10 first");
+    // check whether upgrading from v0.9 to v0.12


from <=0.10 to 0.12

qiaojialin

There is not a thread to do the periodic Channel.force(true) for MLogWriter, is this meet our expectation?

qiaojialin · 2020-12-14T05:40:55Z

server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfig.java

+   * Size of log buffer for every MetaData operation. If the size of a MetaData operation plan
+   * is larger than this parameter, then the MetaData operation plan will be rejected by MManager.
+   */
+  private int mlogBufferSize = 1024 * 1024;


make this consistent with iotdb-engine.properties?

There is not a thread to do the periodic Channel.force(true) for MLogWriter, is this meet our expectation?

Not，just when we use thread pool to write, and we call shutdown to interrupt the thread, see this
https://stackoverflow.com/questions/1161297/why-are-we-getting-closedbyinterruptexception-from-filechannel-map-in-java-1-6

qiaojialin · 2020-12-14T05:44:30Z

server/src/main/java/org/apache/iotdb/db/writelog/io/LogWriter.java

    logFile = SystemFileFactory.INSTANCE.getFile(logFilePath);
+    this.forcePeriodInMs = forcePeriodInMs;
+
+    if (channel == null) {


this is always true, said by idea..

yes, but I think add a check is not redundancy

qiaojialin · 2020-12-14T05:44:37Z

server/src/main/java/org/apache/iotdb/db/writelog/io/LogWriter.java

    this.logFile = logFile;
+    this.forcePeriodInMs = forcePeriodInMs;
+
+    if (channel == null) {


this is always true, said by idea..

qiaojialin · 2020-12-14T05:47:37Z

server/src/main/java/org/apache/iotdb/db/metadata/logfile/MLogWriter.java

+    IoTDBDescriptor.getInstance().getConfig().getMlogBufferSize());
+
+  // we write log to channel every time, so we need not to call channel.force every time
+  private static final long DUMMY_FLUSH_TIME = 100;


The function of this field is the same with a boolean

qiaojialin · 2020-12-14T05:54:36Z

server/src/main/java/org/apache/iotdb/db/writelog/io/LogWriter.java

  }

-  public LogWriter(File logFile) {
+  public LogWriter(File logFile, long forcePeriodInMs) throws FileNotFoundException {


The actual usage of 'forcePeriodInMs' parameter is a boolean 'forceAtEachWrite'

Not that, wal use this to optimize the performance after hundreds of milliseconds.

server/src/main/java/org/apache/iotdb/db/metadata/logfile/MLogWriter.java

qiaojialin · 2020-12-14T06:31:45Z

server/src/main/java/org/apache/iotdb/db/qp/logical/Operator.java

+    CHANGE_TAG_OFFSET, CHANGE_ALIAS, MNODE,
+    MEASUREMENT_MNODE, STORAGE_GROUP_MNODE;


This could be removed, the operator is only generated in SQL parser.

I use them to unify the wal and mlog, for deserializing the log.

sonarqubecloud · 2020-12-14T10:29:11Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

mychaow requested a review from qiaojialin September 10, 2020 09:01

samperson1997 requested changes Sep 11, 2020

View reviewed changes

HTHou requested changes Sep 11, 2020

View reviewed changes

mychaow force-pushed the mlog branch from e2a9f37 to 6c6e0d4 Compare September 11, 2020 06:09

samperson1997 reviewed Sep 11, 2020

View reviewed changes

server/src/main/java/org/apache/iotdb/db/metadata/logfile/MLogWriter.java Show resolved Hide resolved

mychaow requested review from HTHou and samperson1997 September 11, 2020 06:38

qiaojialin reviewed Sep 14, 2020

View reviewed changes

mychaow force-pushed the mlog branch 2 times, most recently from c9db0fe to 72e188c Compare September 27, 2020 06:35

HTHou added the need test label Oct 10, 2020

mychaow force-pushed the mlog branch from 72e188c to 9ea4a4a Compare October 12, 2020 07:19

mychaow force-pushed the mlog branch 2 times, most recently from 3fd3e5a to 5491c8c Compare October 12, 2020 11:30

HTHou added rel/0.11 and removed rel/0.11 labels Oct 26, 2020

mychaow marked this pull request as draft November 18, 2020 06:16

mychaow force-pushed the mlog branch 2 times, most recently from df0282f to 7a5ad6d Compare November 20, 2020 02:37

mychaow marked this pull request as ready for review November 20, 2020 03:41

mychaow force-pushed the mlog branch from d9e2aaa to 18bb055 Compare November 20, 2020 03:44

mychaow requested a review from qiaojialin November 20, 2020 03:52

mychaow added the Module - Schema MManager label Nov 20, 2020

mychaow force-pushed the mlog branch from 18bb055 to 18db49a Compare November 24, 2020 01:59

samperson1997 requested changes Nov 24, 2020

View reviewed changes

HTHou requested changes Nov 25, 2020

View reviewed changes

HTHou reviewed Nov 25, 2020

View reviewed changes

server/src/main/java/org/apache/iotdb/db/conf/IoTDBConfigCheck.java Outdated Show resolved Hide resolved

HTHou reviewed Nov 25, 2020

View reviewed changes

server/src/main/java/org/apache/iotdb/db/qp/logical/Operator.java Outdated Show resolved Hide resolved

HTHou approved these changes Dec 7, 2020

View reviewed changes

qiaojialin reviewed Dec 8, 2020

View reviewed changes

mychaow force-pushed the mlog branch from 9f50518 to 61c6f6d Compare December 11, 2020 01:49

mychaow added 16 commits December 14, 2020 09:42

[IOTDB-868] fix comma bug for mlog

cf986b9

fix upgrade mlog bug

7799fbe

add mlog parser

6b7bedc

fix sonar warning

68e460e

remove some code smell warn

55ebf5b

fix some snoar warn

f03a6c2

fix some bug

d71acfe

remove some unused import

95516e0

fix some sonar warn

bc0aea4

replace system.out with logger

92930ca

add some suppresswarning

68564cf

remove unused pom

546d870

add upgrade system.properties

c76cb3e

fixed 0.11

31fa1e0

catch exception

103f13a

catch closeInterruptException

a415549

mychaow force-pushed the mlog branch from 1cfdd1e to a415549 Compare December 14, 2020 01:43

fix code smell

67b5e08

qiaojialin reviewed Dec 14, 2020

View reviewed changes

mychaow added 3 commits December 14, 2020 15:51

fix clear bug

06b08e0

unify the conf

13d32dd

change long to boolean

4c71885

qiaojialin approved these changes Dec 14, 2020

View reviewed changes

qiaojialin merged commit a75b722 into apache:master Dec 14, 2020

mychaow deleted the mlog branch December 15, 2020 07:20

fanhualta mentioned this pull request Dec 22, 2020

Fix sync schema bug #2309

Merged

		import java.io.IOException;


		public class OldMLogReader implements AutoCloseable {

		CHANGE_TAG_OFFSET, CHANGE_ALIAS, MNODE,
		MEASUREMENT_MNODE, STORAGE_GROUP_MNODE;

Conversation

mychaow commented Sep 10, 2020

Uh oh!

samperson1997 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qiaojialin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mychaow Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HTHou commented Oct 10, 2020

Uh oh!

mychaow commented Oct 12, 2020

Uh oh!

samperson1997 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiaojialin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mychaow Sep 17, 2020 •

edited

Loading