Move inner classes to standalone classes #1095
Conversation
flyrain
requested review from
MonkeyCanCode,
RussellSpitzer,
adutra,
ashvina,
collado-mike,
dennishuo,
dimas-b,
ebyhr,
eric-maynard,
jackye1995,
jbonofre,
snazy,
takidau and
vvcephei
as code owners
dennishuo
left a comment
dennishuo
left a comment
There was a problem hiding this comment.
Generally seems like a good direction to extract all the Result classes. I don't mind too much which package, but should BaseResult be in the same package?
| import org.apache.polaris.core.persistence.BaseResult; | ||
|
|
||
| /** a set of returned entities result */ | ||
| public class EntitiesResult extends BaseResult { |
There was a problem hiding this comment.
Any reason to keep BaseResult in the persistence package when all the subclasses are moved in here, or should BaseResult also be moved here?
There was a problem hiding this comment.
Imho, it should also be in the persistence package to be really isolated.
There was a problem hiding this comment.
Moved BaseResult to the same package. It's under the persistence package already.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
This refactoring LGTM overall, just one minor comment.
| import org.apache.polaris.core.entity.PolarisEntityType; | ||
| import org.apache.polaris.core.entity.PolarisGrantRecord; | ||
| import org.apache.polaris.core.entity.PolarisPrincipalSecrets; | ||
| import org.apache.polaris.core.persistence.dao.entity.*; |
There was a problem hiding this comment.
Please expand to individual imports
There was a problem hiding this comment.
Can we make spotless check for this?
There was a problem hiding this comment.
Removed the star import.
There was a problem hiding this comment.
yes, we can add a file with something like this in it
<module name="AvoidStarImport"/>
There was a problem hiding this comment.
Looks like spotless cannot do that, we will need a new plugin like this
pluginManager.withPlugin('com.palantir.baseline-checkstyle') {
checkstyle {
toolVersion '9.3'
}
}
.../src/main/java/org/apache/polaris/core/persistence/TransactionWorkspaceMetaStoreManager.java
Outdated
Show resolved
Hide resolved
| import org.apache.polaris.core.entity.PolarisPrivilege; | ||
| import org.apache.polaris.core.entity.PolarisTaskConstants; | ||
| import org.apache.polaris.core.persistence.*; | ||
| import org.apache.polaris.core.persistence.dao.entity.*; |
There was a problem hiding this comment.
just in this case this gets missed.
|
Fixed all star import introduced by this PR. Thanks @dimas-b and @singhpk234 for catching it. There are other places having star import as well. File #1100 to track it. |
| import org.apache.polaris.core.entity.PolarisEntitySubType; | ||
|
|
||
| /** the return for an entity lookup call */ | ||
| public class EntityResult extends BaseResult { |
There was a problem hiding this comment.
Organizationally the change makes sense to me, but to my knowledge this is the first PR that adds the DAO / entity terminology -- potentially a little confusing when we have PolarisBaseEntity and friends.
To clarify, what is going to be the scope of this new package?
There was a problem hiding this comment.
sorry, just saw your message after merging. I was trying to position dao to hold most interfaces, entity, util related to DAO layer. I'm open to any suggestions.
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
6 participants
This PR extracts several inner classes of
PolarisMetastoreManagerinto top-level classes, improving readability and maintainability. After the refactor, thePolarisMetastoreManagerinterface now contains only methods, making it concise and clean. Typically, inner classes are used when their lifecycle depends on the outer class instance. However, these particular classes were independent, being used not only by service layers but also potentially by other database implementations. They are more suitable as top-level classes.