tihom88 commented on code in PR #847:
URL: https://github.com/apache/jackrabbit-oak/pull/847#discussion_r1116512379
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/indexversion/IndexVersionOperation.java:
##########
@@ -75,7 +78,7 @@ public String toString() {
*/
public static List<IndexVersionOperation>
generateIndexVersionOperationList(NodeState rootNode, String parentPath,
List<IndexName> indexNameObjectList, long purgeThresholdMillis) {
- return generateIndexVersionOperationList(rootNode, parentPath,
indexNameObjectList, purgeThresholdMillis, true);
+ return generateIndexVersionOperationList(rootNode, parentPath,
indexNameObjectList, purgeThresholdMillis, true, false);
Review Comment:
Shouldn't there be two different implementations for lucene and elastic
(different classes).
I think we already have an issue to have similar hidden mounts for elastic
index, to make elastic compatible with blue-green deployment
##########
oak-run-commons/src/main/java/org/apache/jackrabbit/oak/indexversion/PurgeOldIndexVersion.java:
##########
@@ -166,13 +175,38 @@ private Iterable<String>
getRepositoryIndexPaths(NodeStore store) throws CommitF
* @param commandlineIndexPaths indexpaths provided by user
* @return returns set of indexpaths which are to be considered for purging
*/
- private Set<String> filterIndexPaths(Iterable<String>
repositoryIndexPaths, List<String> commandlineIndexPaths) {
+ private Set<String> filterIndexPaths(NodeStore store, Iterable<String>
repositoryIndexPaths, List<String> commandlineIndexPaths) {
Set<String> filteredIndexPaths = new HashSet<>();
+ NodeState root = store.getRoot();
+ NodeBuilder rootBuilder = root.builder();
for (String commandlineIndexPath : commandlineIndexPaths) {
for (String repositoryIndexPath : repositoryIndexPaths) {
if
(PurgeOldVersionUtils.isIndexChildNode(commandlineIndexPath,
repositoryIndexPath)
||
PurgeOldVersionUtils.isBaseIndexEqual(commandlineIndexPath,
repositoryIndexPath)) {
- filteredIndexPaths.add(repositoryIndexPath);
+ NodeBuilder nodeBuilder =
PurgeOldVersionUtils.getNode(rootBuilder, repositoryIndexPath);
+ if (nodeBuilder.exists()) {
+ String type =
nodeBuilder.getProperty(TYPE_PROPERTY_NAME).getValue(Type.STRING);
+ // If type = disabled. There can be 2 cases -
+ // 1. It was marked as disabled by this code , in
which case the original type would be moved to orig_type property
+ // 2. It was marked disabled by someone else.
+
+ // Skip the run if (type = disabled and orig_type is
present and not equal to PurgeOldIndexVersion's impl's index type)
+ // OR
+ // (type != disabled and type is not equal to
PurgeOldIndexVersion's impl's index type)
+ if ((TYPE_DISABLED.equals(type) &&
nodeBuilder.getProperty(ORIGINAL_TYPE_PROPERTY_NAME) != null &&
!getIndexType().equals(nodeBuilder.getProperty(ORIGINAL_TYPE_PROPERTY_NAME).getValue(Type.STRING)))
||
+ (!TYPE_DISABLED.equals(type) &&
!getIndexType().equals(type))) {
+ continue;
+ }
+
+ // If type = disabled and orig_type is not set - this
means someone manually marked the index as disabled
+ // We can't determine the original index
implementation in this case - so we let it run for whichever
PurgeOldIndexVersion's impl calls it first
+ // Two scenarios to note here -
+ // 1. ElasticPurgeOldIndexVersion calls this for a
lucene index (deletes the index from oak repo - which we want)
+ // and then in postDeleteOp tries to delete a non
existent remote elastic index - that particular call will simply fail.
+ // 2. LucenePurgeOldIndexVersion calls this for an
elastic index (deletes the index from oak repo)
+ // and then does a NOOP in postDeleteOp - effectively
we are left with a dangling elastic remote index in this case.
+ filteredIndexPaths.add(repositoryIndexPath);
+ }
Review Comment:
I think we can get solve dangling elastic issue in following manner.
In case of disabled indexes both elastic and lucene purge operations should
evaluate and add property in hidden node mentioning the state wrt to both
elastic and lucene purge operations.
e.g. purgeLucene = true and purgeElastic=true on :purge node.
and postDeleteOp we check if both these properties are true we delete the
index, else this index will remain in disabled state.
--
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]
