From fca510d58da609738e2c69eb4932d5ddea36a031 Mon Sep 17 00:00:00 2001 From: morningman Date: Tue, 26 Jul 2022 14:28:53 +0800 Subject: [PATCH 1/3] [fix](image) fix bug that latestValidatedImageSeq may not be the second largest image id When traversing the image files in the meta directory, it cannot be guaranteed to be traversed in the order of imageid size For example, if it traverses the image file with orders like: 3,5,4,1, then latestImageSeq is 5, but latestValidatedImageSeq is 3, which is wrong. --- .../org/apache/doris/persist/Storage.java | 28 ++++------- .../org/apache/doris/persist/StorageTest.java | 47 ++++++++----------- 2 files changed, 28 insertions(+), 47 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/persist/Storage.java b/fe/fe-core/src/main/java/org/apache/doris/persist/Storage.java index eec6904476e778..1c123348287942 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/persist/Storage.java +++ b/fe/fe-core/src/main/java/org/apache/doris/persist/Storage.java @@ -19,6 +19,7 @@ import org.apache.doris.ha.FrontendNodeType; +import com.clearspring.analytics.util.Lists; import com.google.common.base.Preconditions; import com.google.common.base.Strings; import org.apache.logging.log4j.LogManager; @@ -59,8 +60,8 @@ public class Storage { private FrontendNodeType role = FrontendNodeType.UNKNOWN; private String nodeName; private long editsSeq; - private long latestImageSeq; - private long latestValidatedImageSeq; + private long latestImageSeq = 0; + private long latestValidatedImageSeq = 0; private String metaDir; private List editsFileSequenceNumbers; @@ -120,6 +121,7 @@ public void reload() throws IOException { if (children == null) { return; } + List imageIds = Lists.newArrayList(); for (File child : children) { String name = child.getName(); try { @@ -127,8 +129,8 @@ public void reload() throws IOException { && !name.endsWith(".part") && name.contains(".")) { if (name.startsWith(IMAGE)) { long fileSeq = Long.parseLong(name.substring(name.lastIndexOf('.') + 1)); + imageIds.add(fileSeq); if (latestImageSeq < fileSeq) { - latestValidatedImageSeq = latestImageSeq; latestImageSeq = fileSeq; } } else if (name.startsWith(EDITS)) { @@ -141,16 +143,15 @@ public void reload() throws IOException { LOG.warn(name + " is not a validate meta file, ignore it"); } } + // set latestValidatedImageSeq to the second largest image id, or 0 if less than 2 images. + Collections.sort(imageIds); + latestValidatedImageSeq = imageIds.size() < 2 ? 0 : imageIds.get(imageIds.size() - 2); } public int getClusterID() { return clusterID; } - public void setClusterID(int clusterID) { - this.clusterID = clusterID; - } - public String getToken() { return token; } @@ -171,10 +172,6 @@ public String getMetaDir() { return metaDir; } - public void setMetaDir(String metaDir) { - this.metaDir = metaDir; - } - public long getLatestImageSeq() { return latestImageSeq; } @@ -183,14 +180,6 @@ public long getLatestValidatedImageSeq() { return latestValidatedImageSeq; } - public void setLatestImageSeq(long latestImageSeq) { - this.latestImageSeq = latestImageSeq; - } - - public void setEditsSeq(long editsSeq) { - this.editsSeq = editsSeq; - } - public long getEditsSeq() { return editsSeq; } @@ -252,6 +241,7 @@ private void writePropertiesToFile(Properties properties, String fileName) throw } } + // Only for test public void clear() throws IOException { File metaFile = new File(metaDir); if (metaFile.exists()) { diff --git a/fe/fe-core/src/test/java/org/apache/doris/persist/StorageTest.java b/fe/fe-core/src/test/java/org/apache/doris/persist/StorageTest.java index ca17a0006c449c..2610e0b8e319a6 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/persist/StorageTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/persist/StorageTest.java @@ -26,6 +26,7 @@ public class StorageTest { private String meta = "storageTestDir/"; + private static long clusterId = 966271669; public void mkdir() { File dir = new File(meta); @@ -42,11 +43,13 @@ public void mkdir() { } public void addFiles(int image, int edit) { - File imageFile = new File(meta + "image." + image); - try { - imageFile.createNewFile(); - } catch (IOException e) { - e.printStackTrace(); + for (int i = 1; i <= image; i++) { + File imageFile = new File(meta + "image." + i); + try { + imageFile.createNewFile(); + } catch (IOException e) { + e.printStackTrace(); + } } for (int i = 1; i <= edit; i++) { @@ -69,7 +72,7 @@ public void addFiles(int image, int edit) { try { version.createNewFile(); String line1 = "#Mon Feb 02 13:59:54 CST 2015\n"; - String line2 = "clusterId=966271669"; + String line2 = "clusterId=" + clusterId; FileWriter fw = new FileWriter(version); fw.write(line1); fw.write(line2); @@ -89,7 +92,6 @@ public void deleteDir() { file.delete(); } } - dir.delete(); } } @@ -110,40 +112,29 @@ public void testConstruct() { @Test public void testStorage() throws Exception { mkdir(); - addFiles(0, 10); - + addFiles(5, 10); Storage storage = new Storage("storageTestDir"); Assert.assertEquals(966271669, storage.getClusterID()); - storage.setClusterID(1234); - Assert.assertEquals(1234, storage.getClusterID()); - Assert.assertEquals(0, storage.getLatestImageSeq()); + Assert.assertEquals(5, storage.getLatestImageSeq()); + Assert.assertEquals(4, storage.getLatestValidatedImageSeq()); Assert.assertEquals(10, Storage.getMetaSeq(new File("storageTestDir/edits.10"))); - Assert.assertTrue(Storage.getCurrentEditsFile(new File("storageTestDir")) - .equals(new File("storageTestDir/edits"))); + Assert.assertTrue( + Storage.getCurrentEditsFile(new File("storageTestDir")).equals(new File("storageTestDir/edits"))); - Assert.assertTrue(storage.getCurrentImageFile().equals(new File("storageTestDir/image.0"))); + Assert.assertTrue(storage.getCurrentImageFile().equals(new File("storageTestDir/image.5"))); Assert.assertTrue(storage.getImageFile(0).equals(new File("storageTestDir/image.0"))); - Assert.assertTrue(Storage.getImageFile(new File("storageTestDir"), 0) - .equals(new File("storageTestDir/image.0"))); + Assert.assertTrue( + Storage.getImageFile(new File("storageTestDir"), 0).equals(new File("storageTestDir/image.0"))); Assert.assertTrue(storage.getCurrentEditsFile().equals(new File("storageTestDir/edits"))); Assert.assertTrue(storage.getEditsFile(5).equals(new File("storageTestDir/edits.5"))); - Assert.assertTrue(Storage.getEditsFile(new File("storageTestDir"), 3) - .equals(new File("storageTestDir/edits.3"))); + Assert.assertTrue( + Storage.getEditsFile(new File("storageTestDir"), 3).equals(new File("storageTestDir/edits.3"))); Assert.assertTrue(storage.getVersionFile().equals(new File("storageTestDir/VERSION"))); - storage.setLatestImageSeq(100); - Assert.assertEquals(100, storage.getLatestImageSeq()); - - storage.setEditsSeq(100); - Assert.assertEquals(100, storage.getEditsSeq()); - Assert.assertEquals("storageTestDir", storage.getMetaDir()); - storage.setMetaDir("abcd"); - Assert.assertEquals("abcd", storage.getMetaDir()); - storage.setMetaDir("storageTestDir"); storage.clear(); File file = new File(storage.getMetaDir()); Assert.assertEquals(0, file.list().length); From 2f4ca5587a7eac3cf0e0656a5addb42e7b65e2a1 Mon Sep 17 00:00:00 2001 From: morningman Date: Tue, 26 Jul 2022 14:39:03 +0800 Subject: [PATCH 2/3] change template --- .github/PULL_REQUEST_TEMPLATE.md | 34 +++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 6f56b55bac8919..7e4ae879df26dc 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -4,16 +4,36 @@ Issue Number: close #xxx ## Problem Summary: -Describe the overview of changes. - ## Checklist(Required) -1. Does it affect the original behavior: (Yes/No/I Don't know) -2. Has unit tests been added: (Yes/No/No Need) -3. Has document been added or modified: (Yes/No/No Need) -4. Does it need to update dependencies: (Yes/No) -5. Are there any changes that cannot be rolled back: (Yes/No) +1. Type of your changes: + - [ ] Improvement + - [ ] Fix + - [ ] Feature-WIP + - [ ] Feature + - [ ] Doc + - [ ] Refator + - [ ] Others: +2. Does it affect the original behavior: + - [ ] Yes + - [ ] No + - [ ] I don't know +3. Has unit tests been added: + - [ ] Yes + - [ ] No + - [ ] No Need +4. Has document been added or modified: + - [ ] Yes + - [ ] No + - [ ] No Need +5. Does it need to update dependencies: + - [ ] Yes + - [ ] No +6. Are there any changes that cannot be rolled back: + - [ ] Yes + - [ ] No ## Further comments If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, etc... + From 49ac0642267f9fa07be466b253b81b282dce818e Mon Sep 17 00:00:00 2001 From: morningman Date: Tue, 26 Jul 2022 14:44:00 +0800 Subject: [PATCH 3/3] change template --- fe/fe-core/src/main/java/org/apache/doris/persist/Storage.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/persist/Storage.java b/fe/fe-core/src/main/java/org/apache/doris/persist/Storage.java index 1c123348287942..d8fc041affc3fc 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/persist/Storage.java +++ b/fe/fe-core/src/main/java/org/apache/doris/persist/Storage.java @@ -19,9 +19,9 @@ import org.apache.doris.ha.FrontendNodeType; -import com.clearspring.analytics.util.Lists; import com.google.common.base.Preconditions; import com.google.common.base.Strings; +import com.google.common.collect.Lists; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger;