-
Notifications
You must be signed in to change notification settings - Fork 199
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
Persistence Refactor POC #1011
base: main
Are you sure you want to change the base?
Persistence Refactor POC #1011
Conversation
import org.apache.polaris.core.entity.PolarisEntityCore; | ||
import org.apache.polaris.core.persistence.dao.NamespaceDao; | ||
|
||
public class PostgresNamespaceDao implements NamespaceDao { |
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.
We could move them into different module if necessary.
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.
Yes, agree, FDB and PostgreSQL should be in their own module.
*/ | ||
package org.apache.polaris.core.persistence.dao; | ||
|
||
public interface CatalogDao {} |
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.
For persistence DAO, my idea was more to have concrete DAO definition to help the implementer with associated record. The DAO deals with storage operation using a record
carriage (to avoid coupliing with entity).
Something like:
public interface CatalogDao {} | |
public record CatalogRecord(String id, String name, String location, ...) { | |
... | |
} |
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'm not sure about this. We should refactor the existing service entities instead of creating a new set of them as I said in the mailing list.
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.
@flyrain you are right for DAO, my point is more for using entity in the DAO. I would rather use a record in the DAO to decouple from Entity: it will force us to decouple service and storage layers, and the record should be "obvious" for DAO implementer.
import org.apache.polaris.core.entity.PolarisEntityCore; | ||
|
||
public interface NamespaceDao { | ||
void save(NamespaceEntity namespace, List<PolarisEntityCore> catalogPath); |
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 think it's a good idea to have operation definitions in the DAO. The DAO could just a record
(public record NamespaceDao
), the storage logic for a DAO is in the persistence implementation.
The idea of DAO is to "map" business logic object to persistence object, I think it's a paradigm we should keep here.
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.
The idea of DAO is to "map" business logic object to persistence object
To be clear, NamespaceEntity
and catalogPath
here should service-layer entities. I put them here to demo how it works with existing code. But they are not necessary to be the final form. The idea of the DAO is to provide an interface so that the impl. can do two-way conversion like the following, while keep the interface only with upper-layer entities.
- service entities -> persistence entities
- persistence entities -> service entities
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.
Yes, it makes sense. As said in previous comment, the DAO as storage operation definition can do the mapping but I think he should use only storage records (persistence entities) that can be converted to service entitiy. Seperation here is welcome.
...ice/common/src/main/java/org/apache/polaris/service/persistence/fdb/dao/FdbNamespaceDao.java
Show resolved
Hide resolved
import org.apache.polaris.core.entity.PolarisEntityCore; | ||
import org.apache.polaris.core.persistence.dao.NamespaceDao; | ||
|
||
public class PostgresNamespaceDao implements NamespaceDao { |
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.
Yes, agree, FDB and PostgreSQL should be in their own module.
.../src/main/java/org/apache/polaris/service/persistence/postgres/dao/PostgresNamespaceDao.java
Show resolved
Hide resolved
...ice/common/src/main/java/org/apache/polaris/service/persistence/fdb/dao/FdbNamespaceDao.java
Show resolved
Hide resolved
The regrest test failure should be fixed by #1015. |
Experiment:
Benefits:
Next Steps:
Please check the detailed design doc: https://docs.google.com/document/d/1Vuhw5b9-6KAol2vU3HUs9FJwcgWtiVVXMYhLtGmz53s/edit?usp=sharing