Skip to content
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

Sync Tool registers 2 tables, RO and RT Tables #210

Merged
merged 1 commit into from
Jun 28, 2017

Conversation

n3nash
Copy link
Contributor

@n3nash n3nash commented Jun 27, 2017

No description provided.

@n3nash n3nash requested a review from prazanna June 27, 2017 16:34
//sync a RO table for MOR
syncHoodieTable(false);
String originalTableName = cfg.tableName;
cfg.tableName = cfg.tableName + SUFFIX_REALTIME_TABLE;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another way to do this is to add an optional config parameter to HiveSyncConfig (only for MOR type of tables) and then refactor parts of HiveSyncTool and HiveHoodieClient to use it conditionally for MOR table types. I feel this keeps conditional clauses away from HiveHoodieClient,HiveSyncTool.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you leave a todo - its always best to have a possible config

@n3nash n3nash self-assigned this Jun 27, 2017
Copy link
Contributor

@prazanna prazanna left a comment

Choose a reason for hiding this comment

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

Looks okay to me overall. Few minor comments.

//sync a RO table for MOR
syncHoodieTable(false);
String originalTableName = cfg.tableName;
cfg.tableName = cfg.tableName + SUFFIX_REALTIME_TABLE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you leave a todo - its always best to have a possible config

switch(hoodieHiveClient.getTableType()) {
case COPY_ON_WRITE:
syncHoodieTable(false);
hoodieHiveClient.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do this once outside the switch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added one to the default case. There should be no scenario where we reach outside the switch without an exception (in which case it doesn't there).

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant why repeat this on every case instead of doing it once outside the switch once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, should've been obvious.

// TODO - RO Table for MOR only after major compaction (UnboundedCompaction is default for now)
hoodieHiveClient.createTable(schema, HoodieInputFormat.class.getName(),
MapredParquetOutputFormat.class.getName(), ParquetHiveSerDe.class.getName());
} else {
// create RT Table
Copy link
Contributor

Choose a reason for hiding this comment

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

remove comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed.

@n3nash n3nash force-pushed the hive_register_2_tables branch from 4488228 to 7fe8623 Compare June 27, 2017 23:27
@n3nash n3nash force-pushed the hive_register_2_tables branch from 7fe8623 to 36a1947 Compare June 28, 2017 04:10
@n3nash
Copy link
Contributor Author

n3nash commented Jun 28, 2017

@prazanna I've addressed all the comments.

@prazanna prazanna requested a review from a team June 28, 2017 22:39
@prazanna prazanna merged commit e5d9b81 into apache:master Jun 28, 2017
vinishjail97 pushed a commit to vinishjail97/hudi that referenced this pull request Dec 15, 2023
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.

2 participants