-
Notifications
You must be signed in to change notification settings - Fork 690
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix TestReplicationHandler #3142
Fix TestReplicationHandler #3142
Conversation
@mlbiscoc I should have reminded/and or run the nightly tests when I was reviewing the path stuff.. Let's remember to do that as we continue! |
Thanks @HoustonPutman for looking at these, and sorry I didn't catch them, as I think I reviewed both of those PR's. Is there some more unit testing that we need that would have prevented these regressions? |
@@ -1037,8 +1036,7 @@ private int indexDirCount(String ddir) { | |||
@Override | |||
public boolean accept(File dir, String name) { | |||
File f = new File(dir, name); | |||
return f.isDirectory() | |||
&& !SolrSnapshotMetaDataManager.SNAPSHOT_METADATA_DIR.equals(name); | |||
return f.isDirectory() && !name.startsWith("snapshot"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems so "magical" that you have to know about a "snapshot".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, it's just in the error output that the snapshot is there. I know no magic 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We should have another crave job that runs the nightly tests. I looked into it, but I don't know where our crave config lives, so I don't know how to enable that. |
I raised the topic on the dev list but FYI I'm our resident Crave expert. |
@dsmiley do you have any opinion on adding the snapshot directories to the filter? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't recall. I used git blame and see that I was reviewing the contribution of a contributor "Hrishikesh Gadre" in SOLR-9269. Back then we used subversion; no contributor metadata in the commit message.
Change seems fine I guess. Basically you loosened from equals to startsWith.
Addresses a combination of SOLR-17306 and SOLR-17548. (cherry picked from commit a5b0b98)
Addresses a combination of SOLR-17306 and SOLR-17548. (cherry picked from commit a5b0b98)
Got it. Thanks @HoustonPutman for fixing the regression! |
This actually stems from two commits on the same day, kind of eerie.
SOLR-17306
This uncommented out:
Apparently when using the StandardDirectoryFactory, we see core snapshot directories when we don't see them using the default DirectoryFactory. The code already accounted for snapshot metadata, but not snapshots themselves. I updated the check to ignore any snapshot directory when counting index directories. @dsmiley, you wrote the snapshot metadata filtering, so can you check this for me?
SOLR-17548
In
ReplicationTestHelper
, this changedto
However, this forgot to allow for overwrites, as was done in other cases in this PR. Probably because nightly tests are usually forgotten.