Skip to content

Add import command for data loader CLI #2618

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

Merged
merged 33 commits into from
May 28, 2025
Merged

Conversation

inv-jishnu
Copy link
Contributor

Description

In this PR I am adding data loader import command and command options for data import.

Related issues and/or PRs

Please review this PR once the below PRs is merged and latest changes from master are merged to this branch

Changes made

I have added the import command options and import command as a CLI command for data import

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes.
  • Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
  • Tests (unit, integration, etc.) have been added for the changes.
  • My changes generate no new warnings.
  • Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

This is the third PR for data loader CLI.

Release notes

NA

@inv-jishnu inv-jishnu added the enhancement New feature or request label Apr 30, 2025
@inv-jishnu inv-jishnu self-assigned this Apr 30, 2025
@inv-jishnu inv-jishnu marked this pull request as draft April 30, 2025 12:15
Base automatically changed from feat/data-loader/cli-export-2 to master May 22, 2025 05:58
Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

Left a few comments. PTAL!

Comment on lines 98 to 99
TableMetadataService tableMetadataService =
new TableMetadataService(storageFactory.getStorageAdmin());
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should close the DistributedStorageAdmin instance after use.

DistributedStorageAdmin admin = storageFactory.getStorageAdmin();

// ...

admin.close();

If there are other parts that need the same fix, could you take care of them in a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you.
I will update the code to close it and will add necessary changes to close other instances in another PR.

}
if (importOptions.getLogMode().equals(LogMode.SPLIT_BY_DATA_CHUNK)) {
importManager.addListener(new SplitByDataChunkImportLogger(config, logWriterFactory));
} else importManager.addListener(new SingleFileImportLogger(config, logWriterFactory));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small nit:

Suggested change
} else importManager.addListener(new SingleFileImportLogger(config, logWriterFactory));
} else {
importManager.addListener(new SingleFileImportLogger(config, logWriterFactory));
}

File configFile = new File(configFilePath);
StorageFactory storageFactory = StorageFactory.create(configFile);
TableMetadataService tableMetadataService =
new TableMetadataService(storageFactory.getStorageAdmin());
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this DistributedStorageAdmin be closed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I will update the code to close it once the use is completed.

importProcessorFactory,
ScalarDbMode.TRANSACTION,
null,
scalarDbTransactionManager.getDistributedTransactionManager());
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this instance be closed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently No. I will created another PR to make necessary changes to confirm that both transaction manager and storage are closed once the process is completed. I will make this change in core part as we can confirm the import process completion in the import manager class.

importOptions,
importProcessorFactory,
ScalarDbMode.STORAGE,
scalarDbStorageManager.getDistributedStorage(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I will update it as mentioned in the above comment,

}
if (importOptions.getLogMode().equals(LogMode.SPLIT_BY_DATA_CHUNK)) {
importManager.addListener(new SplitByDataChunkImportLogger(config, logWriterFactory));
} else importManager.addListener(new SingleFileImportLogger(config, logWriterFactory));
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 wrap this with braces?

@inv-jishnu
Copy link
Contributor Author

@brfrn169 san, @komamitsu san,
I have updated and added changes as per feedback.
I have used a try with resource to close DistributedStorageAdmin used in TableMetadataService part. I will create another PR to ensure the transaction and storage objects are closed properly.
Please take a look at this PR when you get a chance.
Thank you

@inv-jishnu inv-jishnu requested review from komamitsu and brfrn169 May 26, 2025 11:49
Copy link
Contributor

@komamitsu komamitsu left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@brfrn169 brfrn169 requested a review from ypeckstadt May 28, 2025 02:02
Copy link
Contributor

@ypeckstadt ypeckstadt left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

Copy link
Contributor

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@ypeckstadt ypeckstadt merged commit 95b8379 into master May 28, 2025
55 checks passed
@ypeckstadt ypeckstadt deleted the feat/data-loader/cli-import-2 branch May 28, 2025 08:18
feeblefakie pushed a commit that referenced this pull request May 28, 2025
Co-authored-by: Peckstadt Yves <[email protected]>
Co-authored-by: Toshihiro Suzuki <[email protected]>
feeblefakie pushed a commit that referenced this pull request May 28, 2025
Co-authored-by: Peckstadt Yves <[email protected]>
Co-authored-by: Toshihiro Suzuki <[email protected]>
feeblefakie pushed a commit that referenced this pull request May 28, 2025
Co-authored-by: Peckstadt Yves <[email protected]>
Co-authored-by: Toshihiro Suzuki <[email protected]>
feeblefakie pushed a commit that referenced this pull request May 28, 2025
Co-authored-by: Peckstadt Yves <[email protected]>
Co-authored-by: Toshihiro Suzuki <[email protected]>
feeblefakie pushed a commit that referenced this pull request May 28, 2025
Co-authored-by: Peckstadt Yves <[email protected]>
Co-authored-by: Toshihiro Suzuki <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants