HDDS-10568. When the ldb command is executed, it is output by line by jianghuazhu · Pull Request #6420 · apache/ozone

jianghuazhu · 2024-03-22T09:36:58Z

What changes were proposed in this pull request?

When executing the ldb command, if the data is very large, a very large file will be generated, which is not friendly. This pr will add a new function that can control the maximum number of records allowed to be saved in each file.
Details:
HDDS-10568

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-10568

How was this patch tested?

jianghuazhu · 2024-03-22T09:43:50Z

The new command can be combined with --pre-file-records, using:
./bin/ozone debug ldb --db=/xxxx/xxxx/xxxx/om.db scan --column_family=fileTable --out=/xxxx/xxxx/xxxx/om/data/tmp_fileTable --pre-file-records=1000000

Effective file result set:

jianghuazhu · 2024-03-22T09:47:24Z

ci :
https://github.com/jianghuazhu/ozone/actions/runs/8387158994

Can you help review this pr, @adoroszlai @xichen01 .
Thanks.

xichen01

Thanks for working on this, few comments to handle

xichen01 · 2024-03-22T15:32:59Z

- return displayTable(iterator, dbColumnFamilyDef, out, schemaV3);
+ while (iterator.get().isValid()) {
+ try (PrintWriter out = new PrintWriter(new BufferedWriter(
+ new PrintWriter(fileName + fileSuffix, UTF_8.name())))) {


If preFileRecords is not specified, we'd better make the filename the same as the previous filename (without fileSuffix)

xichen01 · 2024-03-22T15:33:10Z

 batch = new ArrayList<>(batchSize);
 sequenceId++;
 }
+ if ((preFileRecords > -1) && (count >= preFileRecords)) {


Seems like the ldb will generate unlimited empty file If the preFileRecords is zero.

xichen01 · 2024-03-22T15:42:14Z

+ @CommandLine.Option(names = {"--pre-file-records"},
+ description = "The number of print records per file.",
+ defaultValue = "-1")
+ private long preFileRecords;


Nit: suggest --max-records-per-file

Thanks for your comment and review, @xichen01 .
I will update soon.

Please also rename preFileRecords to recordsPerFile.
(pre means "before")

jianghuazhu · 2024-03-25T05:40:53Z

Can you help review this PR again, @xichen01 ?
Thanks.

jianghuazhu · 2024-03-26T02:30:02Z

Can you help review this pr, @kerneltime @errose28 .
Thanks.

xichen01 · 2024-03-26T08:10:38Z

+ }
+ fileSuffix++;
+ }
+ } else {


Perhaps we can simplify this if... else
Like:
//... String fileNameXXX = preFileRecords > 0 ? fileName + fileSuffix++ : fileName; //... new PrintWriter(fileNameXXX, UTF_8.name())

errose28

Thanks for the improvement @jianghuazhu. I think the idea is solid since just using split on a stdout stream may produce individual files that are not valid json. Let's add some tests to TestLDBCli to make sure we have all the corner cases around various flag combinations working.

errose28 · 2024-03-26T22:04:47Z

 private int threadCount;

+ @CommandLine.Option(names = {"--max-records-per-file"},
+ description = "The number of print records per file.",


Suggested change
description = "The number of print records per file.",
description = "The number of records to print per file.",

errose28 · 2024-03-26T22:45:05Z

+ if ((preFileRecords > 0) && (count >= preFileRecords)) {
+ break;
+ }


What's the expected behavior when this new --max-records-per-file flag is used without --out? Right now it looks like the choice that stdout is considered "one file" and so this flag overrides the --length option:
# The DB here has many more than 3 entries $ ./ozone debug ldb --db=om.db scan --column_family=fileTable -l3 --max-records-per-file=2 | jq '.[].keyName' | wc -l 2 $ ./ozone debug ldb --db=om.db scan --column_family=fileTable -l2 --max-records-per-file=3 | jq '.[].keyName' | wc -l 2
Maybe we should disallow --max-records-per-file without --out.

-l is also broken with this new option and I got a bit of a surprise trying to test this 😄 I would have expected 5 files, here not 57 thousand.
$ ./ozone debug ldb --db=om.db scan --column_family=fileTable -l10 --max-records-per-file=2 --out=foo ^C $ ls -l | grep foo | wc -l 57343

Thank you for your comment and review.
I will update soon.

-l is also broken with this new option and I got a bit of a surprise trying to test this 😄 I would have expected 5 files, here not 57 thousand.
$ ./ozone debug ldb --db=om.db scan --column_family=fileTable -l10 --max-records-per-file=2 --out=foo ^C $ ls -l | grep foo | wc -l 57343
When --out is not set, all records are output to stdout.
When --max-records-per-file and -l exist at the same time, --max-records-per-file shall prevail.

adoroszlai · 2024-04-22T11:38:02Z

/pending

github-actions

Marking this issue as un-mergeable as requested.

Please use /ready comment when it's resolved.

Please note that the PR will be closed after 21 days of inactivity from now. (But can be re-opened anytime later...)

/pending

jianghuazhu · 2024-04-26T03:41:25Z

Sorry, I had some other work some time ago.
I will make up for this issue in the near future.

jianghuazhu · 2024-04-27T09:46:14Z

The unit tests run fine on my local machine:

Can you help me look at this PR again, @errose28 @xichen01 .
Thanks.

jianghuazhu · 2024-04-29T02:56:20Z

https://github.com/apache/ozone/actions/runs/8849501009/job/24349282946?pr=6420
In CI, it is prompted that File-related operations cannot be performed, such as mkdirs().
Is it because it has something to do with our testing environment?
@errose28 @kerneltime @adoroszlai , do you know the reason?

adoroszlai · 2024-04-29T06:34:34Z

In CI, it is prompted that File-related operations cannot be performed, such as mkdirs().

M B RV: Exceptional return value of java.io.File.mkdirs() ignored in org.apache.hadoop.ozone.debug.DBScanner.displayTable(ManagedRocksIterator, DBColumnFamilyDefinition, boolean) At DBScanner.java:[line 232]

https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html#rv-method-ignores-exceptional-return-value-rv-return-value-ignored-bad-practice

ozone/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/debug/DBScanner.java

Lines 228 to 233 in c7012f3

// If there are no parent directories, create them
Filefile = newFile(fileName); 
FileparentFile = file.getParentFile(); 
if (!parentFile.exists()) { 
parentFile.mkdirs(); 
 } 

File.mkdirs() returns false if it could not create the directories. This return value needs to be handled (e.g. exit with error message).

M D NP: Possible null pointer dereference in org.apache.hadoop.ozone.debug.TestLDBCli.testScanWithRecordsPerFile() due to return value of called method Dereferenced at TestLDBCli.java:[line 286]

https://spotbugs.readthedocs.io/en/latest/bugDescriptions.html#np-possible-null-pointer-dereference-due-to-return-value-of-called-method-np-null-on-some-path-from-return-value

ozone/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/debug/TestLDBCli.java

Lines 285 to 286 in c7012f3

FiletmpDir1 = newFile(scanDir1); 
for (FiletmpFile : tmpDir1.listFiles()) { 

File.listFiles returns null if the path is not a directory. Try checking if tmpDir1 is a directory. The same applies to tmpDir2 below.

jianghuazhu · 2024-04-29T11:53:45Z

ci: https://github.com/jianghuazhu/ozone/actions/runs/8876637070
I've updated it.
Can you help me look at this PR again, @errose28 @xichen01 @adoroszlai ?
Thanks.

adoroszlai · 2024-04-29T12:15:51Z

/ready

Blocking review request is removed.

xichen01 · 2024-05-05T18:02:23Z


 private boolean withinLimit(long i) {
- return limit == -1L || i < limit;
+ return recordsPerFile > 0 || limit == -1L || i < limit;


if the recordsPerFile > 0 is true, the subsequent judgments will be short-circuited, including the i < limit then the limit will be invalidated. This is not a expected function.

Thanks @xichen01 for the comment and review.
When recordsPerFile>0, it means that --max-records-per-file has taken effect, and --limit should be ignored at this time.

The --limit is used to limit the total count of records, the --max-records-per-file is used to limit the max records count of specific file.
Such as:
ozone debug ldb ... --limit 10 --max-records-per-file 1 --out result.txt
This command should generate 10 files, like result.txt0, result.txt1, ..., and each of them has 1 record.

I'll update later.

adoroszlai · 2024-06-29T11:59:26Z

/pending "I'll update later"

github-actions

Marking this issue as un-mergeable as requested.

Please use /ready comment when it's resolved.

Please note that the PR will be closed after 21 days of inactivity from now. (But can be re-opened anytime later...)

"I'll update later"

github-actions · 2024-07-21T00:19:19Z

Thank you very much for the patch. I am closing this PR temporarily as there was no activity recently and it is waiting for response from its author.

It doesn't mean that this PR is not important or ignored: feel free to reopen the PR at any time.

It only means that attention of committers is not required. We prefer to keep the review queue clean. This ensures PRs in need of review are more visible, which results in faster feedback for all PRs.

If you need ANY help to finish this PR, please contact the community on the mailing list or the slack channel."

adoroszlai · 2024-11-24T09:43:12Z

Continued in #7467.

xichen01 reviewed Mar 22, 2024
View reviewed changes

xichen01 reviewed Mar 26, 2024
View reviewed changes

errose28 reviewed Mar 26, 2024
View reviewed changes

github-actionsBot previously requested changes Apr 22, 2024
View reviewed changes

github-actionsBot added the pending label Apr 22, 2024

HDDS-10568. When the ldb command is executed, it is output by line
c7012f3

jianghuazhu force-pushed the HDDS-10568 branch from 60df57f to c7012f3Compare April 26, 2024 14:03

jianghuazhu added 4 commits April 29, 2024 15:42

Fixed some exceptions related to CI.
70f342f

Fix some checkstyle.
805e7dd

Fix some checkstyle.
59e1451

Fix some checkstyle.
15efe88

github-actionsBot removed the pending label Apr 29, 2024

xichen01 reviewed May 5, 2024
View reviewed changes

github-actionsBot requested changes Jun 29, 2024
View reviewed changes

github-actionsBot added the pending label Jun 29, 2024

github-actionsBot closed this Jul 21, 2024

	description = "The number of print records per file.",
	description = "The number of records to print per file.",

Uh oh!

Conversation

jianghuazhu commented Mar 22, 2024

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

jianghuazhu commented Mar 22, 2024

Uh oh!

jianghuazhu commented Mar 22, 2024

Uh oh!

xichen01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jianghuazhu commented Mar 25, 2024

Uh oh!

jianghuazhu commented Mar 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

errose28 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Apr 22, 2024

Uh oh!

github-actionsBot left a comment

Choose a reason for hiding this comment

Uh oh!

jianghuazhu commented Apr 26, 2024

Uh oh!

jianghuazhu commented Apr 27, 2024

Uh oh!

jianghuazhu commented Apr 29, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adoroszlai commented Apr 29, 2024

Uh oh!

jianghuazhu commented Apr 29, 2024

Uh oh!

adoroszlai commented Apr 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Jun 29, 2024

Uh oh!

github-actionsBot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actionsBot commented Jul 21, 2024

Uh oh!

adoroszlai commented Nov 24, 2024

Uh oh!

Reviewers

Assignees

jianghuazhu commented Apr 29, 2024•
edited
Loading