steffenvan commented on code in PR #1123: URL: https://github.com/apache/jackrabbit-oak/pull/1123#discussion_r1328420664
########## oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/incrementalstore/MergeIncrementalFlatFileStore.java: ########## @@ -0,0 +1,155 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.jackrabbit.oak.index.indexer.document.incrementalstore; + +import org.apache.jackrabbit.oak.commons.Compression; +import org.apache.jackrabbit.oak.index.indexer.document.flatfile.FlatFileStoreUtils; +import org.apache.jackrabbit.oak.index.indexer.document.flatfile.NodeStateEntryWriter; +import org.apache.jackrabbit.oak.index.indexer.document.flatfile.NodeStateHolder; +import org.apache.jackrabbit.oak.index.indexer.document.flatfile.PathElementComparator; +import org.apache.jackrabbit.oak.index.indexer.document.flatfile.SimpleNodeStateHolder; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.BufferedReader; +import java.io.BufferedWriter; +import java.io.File; +import java.io.IOException; +import java.util.Comparator; +import java.util.Set; + +import static org.apache.jackrabbit.guava.common.base.Preconditions.checkState; + + +public class MergeIncrementalFlatFileStore { + + private final Logger log = LoggerFactory.getLogger(getClass()); + private final File baseFFS; + private final File incrementalFFS; + private final File merged; + private final Set<String> preferredPathElements; + private final Compression algorithm; + + public MergeIncrementalFlatFileStore(Set<String> preferredPathElements, File baseFFS, File incrementalFFS, + File merged, Compression algorithm) { + this.preferredPathElements = preferredPathElements; + this.baseFFS = baseFFS; + this.incrementalFFS = incrementalFFS; + this.merged = merged; + this.algorithm = algorithm; + } + + public void doMerge() throws IOException { Review Comment: I think it's there's a bit too significant amount of logic compacted together in this function. I'd suggest breaking it down for better clarity and maintainability. Here are some specific points: 1. **Variable Naming Clarity:** The distinction between `incLine` and `baseLine` to me isn't immediately clear. I think we should adopt more descriptive names or add comments for clarification. 2. **Comparator Logic:** The logic for comparison: ```compared = comparator.compare(new SimpleNodeStateHolder(baseLine), new SimpleNodeStateHolder(incLine));``` can be encapsulated within its method. From what I can understand the cases are: a. compared < 0 - seems to handle the base line. b. compared > 0 - seems to manage the incremental line. c. Equal lines - there seems to be special handling for matching lines. Each of these cases can be refactored into their dedicated methods for clarity. 3. **File Writing Logic:** There's recurring logic around writing to the file. I think we should refactor it to make it more readable and make it easier to adopt when e.g. we will use the TreeFS. Here are some methods I drafted as an example: ```java private void writeAndAdvanceLine(BufferedWriter writer, BufferedReader reader) throws IOException { writer.write(reader.readLine()); writer.newLine(); } private void writeAndAdvanceIncremental(BufferedWriter writer, BufferedReader incrementalReader, String incLine) throws IOException { writer.write(removeOperand(incLine)); writer.newLine(); incrementalReader.readLine(); } private void validateAndWriteIncrementalLine(BufferedWriter writer, BufferedReader incrementalReader, String incLine) throws IOException { String operand = getOperand(incLine); if (MODIFY_OPERAND.equals(operand) || DELETE_OPERAND.equals(operand)) { log.error("incremental ffs should not have modify and delete operands: {}", incLine); throw new RuntimeException("incremental ffs should not have modify and delete operands" + incLine); } writer.write(removeOperand(incLine)); writer.newLine(); incrementalReader.readLine(); } ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
