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

Export options and basic export report dto #2397

Merged
merged 16 commits into from
Dec 19, 2024

Conversation

inv-jishnu
Copy link
Contributor

@inv-jishnu inv-jishnu commented Dec 11, 2024

Description

Added export options builder and export report class

Related issues and/or PRs

PR dependency

Changes made

Added export options builder and export report class

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 PR is part of the on-going process of merging all ScalarDB Data Loader code into the main repository.

Release notes

NA

@inv-jishnu inv-jishnu added the enhancement New feature or request label Dec 11, 2024
@inv-jishnu inv-jishnu marked this pull request as draft December 11, 2024 10:39
@ypeckstadt ypeckstadt marked this pull request as ready for review December 17, 2024 08:19
@@ -34,4 +34,9 @@
<Bug pattern="ODR_OPEN_DATABASE_RESOURCE"/>
</Or>
</Match>
<!-- Ignore mutable object exposure warnings(caused by Lombok) for all classes in dataloader.core -->
<Match>
Copy link
Contributor

Choose a reason for hiding this comment

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

Lombok does not work well with Spotbugs. To avoid having to disable the bug patterns for all projects, the rules are only disabled for the com.scalar.db.dataloader.core.* package.

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! 👍

* exported file. AtomicInteger is used because it is thread safe and this parameter is a counter
* which is updated inside threads
*/
private final AtomicInteger exportedRowCount;
Copy link
Contributor

Choose a reason for hiding this comment

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

[minor] This can be initialized here.

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 have moved the initialisation as suggested (I have changed AtomicInteger to LongAdder based on Suzuki-san suggestion).
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!

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 couple of comments. Please take a look when you have time!

* exported file. AtomicInteger is used because it is thread safe and this parameter is a counter
* which is updated inside threads
*/
private final AtomicInteger exportedRowCount;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 for the suggestion.
I have replaced AtomicInteger with LongAdder.

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

public class ExportReportTest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public class ExportReportTest {
class ExportReportTest {

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 have updated it as suggested.
Thank you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like it's not updated?

Copy link
Contributor Author

@inv-jishnu inv-jishnu Dec 19, 2024

Choose a reason for hiding this comment

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

@brfrn169
My apologies, I think I missed to commit the file. I have pushed the change now.

@inv-jishnu inv-jishnu requested a review from brfrn169 December 18, 2024 10:39
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!

@ypeckstadt ypeckstadt merged commit 05251a2 into master Dec 19, 2024
48 checks passed
@ypeckstadt ypeckstadt deleted the feat/data-loader/export-data-1 branch December 19, 2024 06:58
feeblefakie pushed a commit that referenced this pull request Dec 19, 2024
feeblefakie pushed a commit that referenced this pull request Dec 19, 2024
feeblefakie pushed a commit that referenced this pull request Dec 19, 2024
feeblefakie pushed a commit that referenced this pull request Dec 19, 2024
feeblefakie pushed a commit that referenced this pull request Dec 19, 2024
feeblefakie pushed a commit that referenced this pull request Dec 20, 2024
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