Skip to content

SOLR-16040: Fix split packages in hdfs module#688

Merged
risdenk merged 1 commit intoapache:mainfrom
risdenk:SOLR-16040
Feb 23, 2022
Merged

SOLR-16040: Fix split packages in hdfs module#688
risdenk merged 1 commit intoapache:mainfrom
risdenk:SOLR-16040

Conversation

@risdenk
Copy link
Contributor

@risdenk risdenk commented Feb 22, 2022

https://issues.apache.org/jira/browse/SOLR-16040

Its broken down into multiple commits just as I worked through it. The only weird commit is ce7d256c89eca082a3ebe59595a9d61e098871bc where there were a bunch of changes needed to open up update and transaction classes to allow the new hdfs package to work. Everything before was package private so it worked in the same package.

Tested with:

./gradlew check
./gradlew test --tests "*hdfs*" --tests "*Hdfs*" --tests "*HDFS*" --tests "*Hadoop*" -Ptests.jvms=4 -Ptests.jvmargs=-XX:TieredStopAtLevel=1 -Ptests.nightly=true -Ptests.awaitsfix=false -Ptests.badapples=false -Ptests.file.encoding=UTF-8

@risdenk risdenk self-assigned this Feb 22, 2022
@dsmiley
Copy link
Contributor

dsmiley commented Feb 22, 2022

While you're at it, could you please rename pertinent tests that aren't using camel case for Hdfs like HDFSCollectionsAPITest ? Maybe non-test classes too. This is the standard I've seen most projects gravitate towards instead of it being haphazard.

Copy link
Contributor

@janhoy janhoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love to see how eating our own dogfood improves our base classes by detecting private access for crucial methods and variables, that would prevent users to create their own custom plugins for the same.

Did a quick browse only.

@risdenk
Copy link
Contributor Author

risdenk commented Feb 22, 2022

@dsmiley I think I fixed all the class names in 1f47ffe

@risdenk
Copy link
Contributor Author

risdenk commented Feb 23, 2022

@dsmiley fixed Filesystem in f5249ad

@risdenk risdenk requested a review from janhoy February 23, 2022 01:49
Copy link
Contributor

@HoustonPutman HoustonPutman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me with a changelog entry. Though I can't speak for the SolrResourceLoader class.

@risdenk risdenk merged commit 7303361 into apache:main Feb 23, 2022
@risdenk risdenk deleted the SOLR-16040 branch February 23, 2022 18:48
@HoustonPutman
Copy link
Contributor

Did anything else get overridden in the force push?

@risdenk
Copy link
Contributor Author

risdenk commented Feb 23, 2022

@HoustonPutman the force push wasn't the issue. I didn't pull the branch before making local changes to add the solr/CHANGES.txt. The only change made through the UI was your documentation change.

@HoustonPutman
Copy link
Contributor

Ahh ok, thanks for explaining 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants