From 2cb651f4bf0b0e9795a8ea5eeebc43e5f72a0720 Mon Sep 17 00:00:00 2001 From: Mingyu Chen Date: Tue, 26 Jul 2022 22:35:31 +0800 Subject: [PATCH] [fix](image) fix bug that latestValidatedImageSeq may not be the second largest image id (#11201) * [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. --- .github/PULL_REQUEST_TEMPLATE.md | 34 +++++++++++--- .../org/apache/doris/persist/Storage.java | 29 ++++-------- .../org/apache/doris/persist/StorageTest.java | 47 ++++++++----------- 3 files changed, 55 insertions(+), 55 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... + 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 8eb0336d716b82..3f2f5bd5073f0f 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 @@ -21,7 +21,7 @@ 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; @@ -60,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; @@ -121,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 { @@ -128,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)) { @@ -142,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; } @@ -172,10 +172,6 @@ public String getMetaDir() { return metaDir; } - public void setMetaDir(String metaDir) { - this.metaDir = metaDir; - } - public long getLatestImageSeq() { return latestImageSeq; } @@ -184,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; } @@ -253,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);