-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[HUDI-3654] Add new module hudi-metaserver
#5064
Conversation
Add catalogName parameter to MetadataStore interface method |
great feature !!! |
@xiarixiaoyao : can you review this when you get a chance. I have assigned it to myself as well. So, will try to review in a weeks time. |
@minihippo could you pls rebase the code and run azure again, thanks |
2dfad79
to
51c78ac
Compare
The CI environment lacks thrift under |
hudi-metastore/src/main/java/org/apache/hudi/common/table/HoodieTableMetastoreClient.java
Outdated
Show resolved
Hide resolved
hudi-metastore/src/main/java/org/apache/hudi/common/table/HoodieTableMetastoreClient.java
Outdated
Show resolved
Hide resolved
...astore/src/main/java/org/apache/hudi/common/table/timeline/HoodieMetastoreBasedTimeline.java
Outdated
Show resolved
Hide resolved
@xiarixiaoyao It seems that the code review doesn't finish, right? |
hudi-metastore/src/main/java/org/apache/hudi/metastore/HoodieMetastoreServer.java
Outdated
Show resolved
Hide resolved
hudi-metastore/src/main/java/org/apache/hudi/metastore/HoodieMetastoreServer.java
Outdated
Show resolved
Hide resolved
.newProxyInstance(HoodieMetaStoreProxyHandler.class.getClassLoader(), | ||
new Class[]{ThriftHoodieMetastore.Iface.class}, proxyHandler); | ||
ThriftHoodieMetastore.Processor processor = new ThriftHoodieMetastore.Processor(proxy); | ||
TServerTransport serverTransport = new TServerSocketWrapper(9090); |
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.
Let's make the port configurable
.newProxyInstance(HoodieMetaStoreProxyHandler.class.getClassLoader(), | ||
new Class[]{ThriftHoodieMetastore.Iface.class}, proxyHandler); | ||
ThriftHoodieMetastore.Processor processor = new ThriftHoodieMetastore.Processor(proxy); | ||
TServerTransport serverTransport = new TServerSocketWrapper(9090); |
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.
Let's make the port configurable
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 need a. jcommander to do it, will fix later
hudi-metastore/src/main/java/org/apache/hudi/metastore/client/HoodieMetastoreClientImp.java
Outdated
Show resolved
Hide resolved
hudi-metastore/src/main/java/org/apache/hudi/metastore/HoodieMetastoreServer.java
Outdated
Show resolved
Hide resolved
Throwable err = null; | ||
do { | ||
try { | ||
Object res = method.invoke(client, args); |
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 will be better to log time cost
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.
Should us involve metrics to do this or just log?
Throwable err = null; | ||
do { | ||
try { | ||
Object res = method.invoke(client, args); |
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 will be better to log time cost
hudi-metastore/src/main/java/org/apache/hudi/metastore/service/HoodieMetastoreService.java
Outdated
Show resolved
Hide resolved
import java.nio.ByteBuffer; | ||
import java.util.List; | ||
|
||
public class HoodieMetastoreService implements ThriftHoodieMetastore.Iface, Serializable { |
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.
pls add doc for all new class
location_uri VARCHAR(512) COMMENT 'database storage path', | ||
name VARCHAR(512) UNIQUE COMMENT 'database name', | ||
owner_name VARCHAR(512) COMMENT 'database owner' , | ||
owner_type VARCHAR(512) COMMENT 'database owner' |
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.
All tables here are added with create_time
and update_time
for easy problem analysis
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.
will fix
|
||
@Override | ||
protected void initMetaClient(HoodieTableType tableType, Properties properties) throws IOException { | ||
properties.put(HoodieTableConfig.NAME.key(), "test"); |
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.
Redundant, repeat with initMetastoreConfig
below.
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.
will fix
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.
+1
@hudi-bot run azure |
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 overall. suggest some minor changes. pls follow up the comments in separate PRs
} | ||
LOG.info(String.format("Table %s.%s doesn't exist, will create it.", db, tb)); | ||
table = new Table(); | ||
table.setDbName(db); |
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.
setDatabaseName()
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.
fix
/** | ||
* A proxy for meta server, accepts all thrift calls and routes them to the corresponding service. | ||
*/ | ||
public class HoodieMetaserverService implements ThriftHoodieMetaserver.Iface, Serializable { |
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.
MetaserverGateway sounds better
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.
fix
// todo: support SSS | ||
SimpleDateFormat sdf = new SimpleDateFormat("yyyyMMddHHmmss"); |
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.
should actually reuse the format from the existing constant. And make millisecond first supported.
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.
fix
} | ||
|
||
@Override | ||
public byte[] getInstantMetadata(long tableId, THoodieInstant instant) throws MetaserverStorageException { |
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 other interface is using Option<byte[]>. we should align them for consistency
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.
fix
* Table entity for store. | ||
*/ | ||
public class TableBean { | ||
private String dbName; |
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.
use databaseName
; pls review all occurrences
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.
fix all
} | ||
|
||
public BatchDaoOperation(String sqlID, Object parameter, String operationType) { | ||
this(null, sqlID, parameter, operationType); |
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.
namespace is nullable? let's avoid passing null around by making Option namespace
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.
delete the useless code
// table related | ||
struct Table { | ||
1: string tableName, | ||
2: string dbName, |
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.
databaseName
comments addressed and planned for follow up changes
Co-authored-by: gengxiaoyu <gengxiaoyu@bytedance.com> Co-authored-by: Raymond Xu <2701446+xushiyan@users.noreply.github.com>
Co-authored-by: gengxiaoyu <gengxiaoyu@bytedance.com> Co-authored-by: Raymond Xu <2701446+xushiyan@users.noreply.github.com>
Change Logs
Add a new module
hudi-metastore
Impact
HoodieMetastoreBasedTimeline
HoodieMetastoreFileSystemView
MetaStore has three parts:
Writing a commit/deltacommit is available, but read is not ready.
Risk level
medium
For test, metastore will start up with an embedded one.
This change added tests and can be verified as follows:
Documentation Update
Contributor's checklist