-
Notifications
You must be signed in to change notification settings - Fork 872
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
maven indexing lucene renovation #3558
Conversation
a6d54a7
to
086d6be
Compare
71170ed
to
f9fabe1
Compare
f9fabe1
to
98d4f33
Compare
It turned out that the reason for But even if it worked before, it is likely still a bug since it could produce false positives if I see this correctly. In any case a big part of the Base64 key space is wasted. The hack 587084e simulates the old behavior and lets all tests pass locally. potential fix: encode with Base32 instead (in lowercase, but this would be 7 chars (1 char longer) or find other solution preferably via lucene, unfortunately maven-indexer has it's NexusAnalyzer hard coded. |
This comment was marked as outdated.
This comment was marked as outdated.
b7d1b18
to
e14588c
Compare
regarding the timeout of Lines 145 to 149 in c084119
post migration, the NPE does not occur. The query appears to work. However, this causes the download+indexing of the maven central repo. Which causes a timeout after 3 minutes. So my question is: was the test originally supposed to download the index? NPE suppression was added here: 57e21b8 suppressed exception for reference (pre-migration):
|
e14588c
to
b1767a0
Compare
This comment was marked as outdated.
This comment was marked as outdated.
dd73403
to
a3e9997
Compare
switched to lowercase base32 encoding since I couldn't find any other way to make it work. index size wise, the base64->base32 switch did not make any noteworthy difference. Both versions however are ~800 MB smaller when compared to the old lucene 5 index (fresh maven central index).
|
a3e9997
to
04fa8fb
Compare
@Override | ||
protected Gav getGavFromPath(IndexingContext context, String repositoryPath, String artifactPath) { | ||
// filter out maven-metadata[-repo_name].xml to avoid IndexOutOfBoundsExceptions in | ||
// org.apache.maven.index.artifact.M2GavCalculator.getSnapshotGav(M2GavCalculator.java:187) | ||
if (artifactPath.endsWith(".xml") && artifactPath.substring(artifactPath.lastIndexOf("/")+1).startsWith("maven-metadata")) { | ||
return null; | ||
} | ||
return super.getGavFromPath(context, repositoryPath, artifactPath); | ||
} |
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.
added a workaround for the IOOBE in maven-indexer.
issue filed under:
https://issues.apache.org/jira/browse/MINDEXER-144
Hi @mbien, I'm not sure my issue is related to your task but the Apache NetBeans indexer was not able to index Apache Archiva repository for a long time. It seems that your patch make it working again. |
@ebarboni thanks Eric for testing. That is good news. This PR was a little bit of a minefield :) |
for future reference, the assertion error in BooleanQuery.hashcode() attached below. netbeans/java/maven.indexer/src/org/netbeans/modules/maven/indexer/NexusRepositoryIndexerImpl.java Lines 1451 to 1457 in 04fa8fb
Problem can be mitigated by not changing the rewrite method to lucene issue filed: PRs:
Issue can be triggered via UI, right click on project dependencies -> "Add Dependency..." -> use "query:" search field, e.g enter "org.eclipse.jetty".
|
Maven continues to support JDK8 and even older. E.g. Maven itself must be using compatible version of Lucene. I suggest NetBeans to use the same version. |
you mean maven-indexer? Thats exactly what this PR does. Latest 8.x is in use which is the same what maven-indexer 6.1x uses, Compatibility windows are described in the PR text and why this all had to be done in one step (5.x -> 8.x). The encountered problems are listed too. The only potential showstopper remaining (IMO) is the discovered bug in lucene's hashcode logic, although it got quickly fixed thanks to lucene devs after I filed the issue, it doesn't really help us if it won't be backported to 8.x which we are stuck to due to the JDK 8 requirement of this module. |
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.
In general looks good. A quick local test showed, that searching for dependencies (locally by class, remote by artifact/group name). I left a few inline comments though.
@@ -247,7 +247,7 @@ public void performOperation(POMModel model) { | |||
//nbm-maven-plugin | |||
boolean addPlugin = true; | |||
String managedPVersion = null; | |||
String pVersion = MavenNbModuleImpl.getLatestNbmPluginVersion(); | |||
String pVersion = MavenNbModuleImpl.LATEST_NBM_PLUGIN_VERSION; |
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.
Is the change in semantics intended? If I read the method correctly it will really fetch the latest version, while your change switches to a hard coded value. This looks fishy to me.
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.
it is intended, Its this commit: 5b9efc2
explanation is here:
#3558 (comment)
short version:
code swallowed a NPE which basically made the tests test nothing, post upgrade the NPE did not occur and the tests tried to download the maven repo index which caused them to time out
by returning a constant (again, i basically reverted an older commit) and not ignoring the exception, everything works again.
Yes, semantics changed a little to the old behavior before 57e21b8
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.
@oyarzun could you please have a look at this - it is your commit, for which the behavior is changed, so this needs to be reevaluated.
java/maven.indexer/src/org/netbeans/modules/maven/indexer/ClassDependencyIndexCreator.java
Show resolved
Hide resolved
java/maven.indexer/src/org/netbeans/modules/maven/indexer/NexusRepositoryIndexerImpl.java
Outdated
Show resolved
Hide resolved
java/maven.indexer/src/org/netbeans/modules/maven/indexer/NexusRepositoryIndexerImpl.java
Outdated
Show resolved
Hide resolved
c03e479
to
5ebf50f
Compare
java/maven.indexer/src/org/netbeans/modules/maven/indexer/ClassDependencyIndexCreator.java
Outdated
Show resolved
Hide resolved
@matthiasblaesing what is your opinion on this. This is a fairly mission critical area so we really should make sure that everything works before integrating IMO... but we have only a week left till freeze - would be great to get more reviews. |
5ebf50f
to
a8681f7
Compare
@mbien sorry for the late reply: The outstanding question is the handling of the plugin version. Would it be possible to implement something like this:
That way we have the best of both worlds. |
apisupport/maven.apisupport/src/org/netbeans/modules/maven/apisupport/MavenNbModuleImpl.java
Outdated
Show resolved
Hide resolved
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 suggest to get this into master, once it settles down, the earlier, the long we have time to fix bugs ;-)
- added jdk/incubator to jdk class list test. - fix: move maven-indexer to the front in the classpath. - migration had to be done in one step since there are no maven-indexer releases available which support lucene 6 or 7.
lucene is case sensitive, maven-indexer however uses a lower case filter on its hardcoded NexusAnalyzer. This means that most class dependency queries wouldn't return anything. Base32 uses only one case (usually upper case, but we use lower case).
…nloaded. - query latest nbm-version but don't wait for the index download - this reverts some of 57e21b8 why did this work before migration? The code swallowed an exception during junit tests which caused the logic to return a fallback constant as version. Post migration, this exception does not occure and the method would correctly block -> tests time out.
- lambdas, diamonds, overrides, final fields, arm blocks - if chain over Strings -> switch - str.replaceAll -> str.replace for non-regex usages - map.contains + map.get -> map.get + null check - collection sizing where known - deprecations and warning removals - dead code removal - formatting fixes
b4842a1
to
ec298fb
Compare
ok awesome. Travis is working here too. I squashed a few less important commits, i would like to merge the rest without squashing so that it simplifies debugging with git bisections if problems occur. |
Check if the maven repo indexing module can be upgraded from apache lucene 5.x to 9.x.
maven-indexer
to 6.10+maven-indexer
releases compatible with lucene 6.x or 7.x.note:
progress:
using local SNAPSHOT atm)ClassDependencyIndexCreatorTest.testFindClassUsages()
fails with no results found, other tests are green.BooleanQuery.hashCode
/ lucene -> https://issues.apache.org/jira/browse/LUCENE-10431 -> maven indexing lucene renovation #3558 (comment)maven.apisupport
tests were failing since they loaded the wrong lucene jars (3.x) while using loading maven-indexer 6.1:maven.apisupport
tests are now timing out since they decided to download and index the entire maven central repo