thomasmueller commented on code in PR #1189:
URL: https://github.com/apache/jackrabbit-oak/pull/1189#discussion_r1383306559
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/incrementalstore/MergeIncrementalFlatFileStore.java:
##########
@@ -96,6 +96,10 @@ private void basicFileValidation(Compression algorithm,
File... files) {
}
}
+ /* this method is a little verbose but I think this is fine as we are not
getting consistent
+ data from checkpoint diff and we need to handle cases differently.
+
+ */
Review Comment:
```suggestion
/**
* Merges multiple index store files.
*
* This method is a little verbose, but I think this is fine
* as we are not getting consistent data from checkpoint diff
* and we need to handle cases differently.
*/
```
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/incrementalstore/MergeIncrementalFlatFileStore.java:
##########
@@ -113,18 +117,23 @@ private void mergeIndexStoreFiles() throws IOException {
compared = comparator.compare(new
SimpleNodeStateHolder(baseFFSLine), new
SimpleNodeStateHolder(incrementalFFSLine));
if (compared < 0) { // write baseFFSLine in merged file
and advance line in baseFFS
baseFFSLine = writeAndAdvance(writer,
baseFFSBufferedReader, baseFFSLine);
- } else if (compared > 0) { // write incrementalFFSline and
advance line in incrementalFFS
- String[] incrementalFFSParts =
IncrementalFlatFileStoreNodeStateEntryWriter.getParts(incrementalFFSLine);
- if
(!IncrementalStoreOperand.ADD.toString().equals(getOperand(incrementalFFSParts)))
{
- log.warn("Expected operand {} but got {} for
incremental line {}. Merging will proceed as usual, but this needs to be looked
into.",
- IncrementalStoreOperand.ADD,
getOperand(incrementalFFSParts), incrementalFFSLine);
- }
- incrementalFFSLine = writeAndAdvance(writer,
incrementalFFSBufferedReader,
-
getFFSLineFromIncrementalFFSParts(incrementalFFSParts));
+ }
+ // We are adding warn logs instead of checkState e.g.
+ // 1- delete a node
+ // 2- add node at same path.
+ // The incremental FFS with above operations are dumped as
node added instead of modified.
+ else if (compared > 0) { // write incrementalFFSline and
advance line in incrementalFFS
+ incrementalFFSLine =
processIncrementalFFSLine(enumMap, writer, incrementalFFSBufferedReader,
incrementalFFSLine);
} else {
String[] incrementalFFSParts =
IncrementalFlatFileStoreNodeStateEntryWriter.getParts(incrementalFFSLine);
String operand = getOperand(incrementalFFSParts);
switch (enumMap.get(operand)) {
+ case ADD:
+ log.warn("Expected operand {} or {} but got {}
for incremental line {}. Merging will proceed as usual, but this needs to be
looked into.",
Review Comment:
```suggestion
log.warn("Expected operand {} or {} but got
{} for incremental line {}. " +
"Merging will proceed, but this is
unexpected.",
```
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/incrementalstore/MergeIncrementalFlatFileStore.java:
##########
@@ -113,18 +117,23 @@ private void mergeIndexStoreFiles() throws IOException {
compared = comparator.compare(new
SimpleNodeStateHolder(baseFFSLine), new
SimpleNodeStateHolder(incrementalFFSLine));
if (compared < 0) { // write baseFFSLine in merged file
and advance line in baseFFS
baseFFSLine = writeAndAdvance(writer,
baseFFSBufferedReader, baseFFSLine);
- } else if (compared > 0) { // write incrementalFFSline and
advance line in incrementalFFS
- String[] incrementalFFSParts =
IncrementalFlatFileStoreNodeStateEntryWriter.getParts(incrementalFFSLine);
- if
(!IncrementalStoreOperand.ADD.toString().equals(getOperand(incrementalFFSParts)))
{
- log.warn("Expected operand {} but got {} for
incremental line {}. Merging will proceed as usual, but this needs to be looked
into.",
- IncrementalStoreOperand.ADD,
getOperand(incrementalFFSParts), incrementalFFSLine);
- }
- incrementalFFSLine = writeAndAdvance(writer,
incrementalFFSBufferedReader,
-
getFFSLineFromIncrementalFFSParts(incrementalFFSParts));
+ }
+ // We are adding warn logs instead of checkState e.g.
+ // 1- delete a node
+ // 2- add node at same path.
+ // The incremental FFS with above operations are dumped as
node added instead of modified.
+ else if (compared > 0) { // write incrementalFFSline and
advance line in incrementalFFS
+ incrementalFFSLine =
processIncrementalFFSLine(enumMap, writer, incrementalFFSBufferedReader,
incrementalFFSLine);
} else {
String[] incrementalFFSParts =
IncrementalFlatFileStoreNodeStateEntryWriter.getParts(incrementalFFSLine);
String operand = getOperand(incrementalFFSParts);
switch (enumMap.get(operand)) {
+ case ADD:
+ log.warn("Expected operand {} or {} but got {}
for incremental line {}. Merging will proceed as usual, but this needs to be
looked into.",
+ IncrementalStoreOperand.MODIFY,
IncrementalStoreOperand.DELETE, getOperand(incrementalFFSParts),
incrementalFFSLine);
Review Comment:
```suggestion
IncrementalStoreOperand.MODIFY,
IncrementalStoreOperand.DELETE, operand, incrementalFFSLine);
```
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/incrementalstore/MergeIncrementalFlatFileStore.java:
##########
@@ -149,6 +158,39 @@ private void mergeIndexStoreFiles() throws IOException {
}
}
+ private String processIncrementalFFSLine(Map<String,
IncrementalStoreOperand> enumMap, BufferedWriter writer,
+ BufferedReader
incrementalFFSBufferedReader, String incrementalFFSLine) throws IOException {
+ String[] incrementalFFSParts =
IncrementalFlatFileStoreNodeStateEntryWriter.getParts(incrementalFFSLine);
+ String operand = getOperand(incrementalFFSParts);
+ switch (enumMap.get(operand)) {
+ case ADD:
+ incrementalFFSLine = writeAndAdvance(writer,
incrementalFFSBufferedReader,
+
getFFSLineFromIncrementalFFSParts(incrementalFFSParts));
+ break;
+ case MODIFY:
+ // this case should not happen. But in case this happens we
consider modify as addition of node
+ // this implies node is not present in older FFS and in
checkpointdiff this came as modified instead of
+ // node addition.
+ log.warn("Expected operand {} but got {} for incremental line
{}. Merging will proceed as usual, but this needs to be looked into.",
Review Comment:
same as above
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/index/indexer/document/incrementalstore/MergeIncrementalFlatFileStore.java:
##########
@@ -149,6 +158,39 @@ private void mergeIndexStoreFiles() throws IOException {
}
}
+ private String processIncrementalFFSLine(Map<String,
IncrementalStoreOperand> enumMap, BufferedWriter writer,
+ BufferedReader
incrementalFFSBufferedReader, String incrementalFFSLine) throws IOException {
+ String[] incrementalFFSParts =
IncrementalFlatFileStoreNodeStateEntryWriter.getParts(incrementalFFSLine);
+ String operand = getOperand(incrementalFFSParts);
+ switch (enumMap.get(operand)) {
+ case ADD:
+ incrementalFFSLine = writeAndAdvance(writer,
incrementalFFSBufferedReader,
+
getFFSLineFromIncrementalFFSParts(incrementalFFSParts));
+ break;
+ case MODIFY:
+ // this case should not happen. But in case this happens we
consider modify as addition of node
+ // this implies node is not present in older FFS and in
checkpointdiff this came as modified instead of
+ // node addition.
+ log.warn("Expected operand {} but got {} for incremental line
{}. Merging will proceed as usual, but this needs to be looked into.",
+ IncrementalStoreOperand.ADD,
getOperand(incrementalFFSParts), incrementalFFSLine);
+ incrementalFFSLine = writeAndAdvance(writer,
incrementalFFSBufferedReader,
+
getFFSLineFromIncrementalFFSParts(incrementalFFSParts));
+ break;
+ case DELETE:
+ // This case should not happen. If this happens, it means we
don't have any such node in baseFFS
+ // but this node came as deletion of node for an already
non-existing node.
+ // we just skip this node in this case.
+ log.warn("Expected operand {} but got {} for incremental line
{}. Merging will proceed as usual, but this needs to be looked into.",
+ IncrementalStoreOperand.ADD,
getOperand(incrementalFFSParts), incrementalFFSLine);
+ incrementalFFSLine = incrementalFFSBufferedReader.readLine();
+ break;
+ default:
+ log.error("wrong operand in incremental ffs: operand:{},
line:{}", operand, incrementalFFSLine);
Review Comment:
```suggestion
log.error("Wrong operand in incremental ffs: operand {},
line {}", operand, incrementalFFSLine);
```
--
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: dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
