-
Notifications
You must be signed in to change notification settings - Fork 141
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
[#972 ] fix(tez): Add output mapOutputByteCounter metrics #1016
Conversation
Could you polish your pull request description? Could you fill the correct information instead of default information? |
Codecov Report
@@ Coverage Diff @@
## master #1016 +/- ##
============================================
+ Coverage 53.65% 54.81% +1.16%
- Complexity 2520 2521 +1
============================================
Files 382 362 -20
Lines 21647 19289 -2358
Branches 1798 1798
============================================
- Hits 11614 10573 -1041
+ Misses 9328 8083 -1245
+ Partials 705 633 -72
... and 21 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ok 👌
| |
bin_zhang_apply
|
|
***@***.***
|
---- Replied Message ----
| From | ***@***.***> |
| Date | 07/17/2023 22:33 |
| To | apache/incubator-uniffle ***@***.***> |
| Cc | bin41215 ***@***.***>,
Author ***@***.***> |
| Subject | Re: [apache/incubator-uniffle] [#972 ] fix(tez): Add output mapOutputByteCounter metrics (PR #1016) |
Could you polish your pull request description? Could you fill the correct information instead of default information?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Configuration conf = new Configuration(); | ||
FileSystem localFs = FileSystem.getLocal(conf); | ||
Path workingDir = new Path(System.getProperty("test.build.data", | ||
System.getProperty("java.io.tmpdir", "/tmp")), |
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.
Could you use org.junit.jupiter.api.io.TempDir
instead of /tmp
? TempDir can be cleaned automatically. Maybe we don't have the permission of /tmp
in some machine envrionment.
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.
Could you use
org.junit.jupiter.api.io.TempDir
instead of/tmp
? TempDir can be cleaned automatically. Maybe we don't have the permission of/tmp
in some machine envrionment.
there need hadoop.fs.Path but TempDir only support nio.file.Path and io.File
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.
You can use similar code as below.
public void test(@TempDir File tmpDir) throws IOException {
Path workingDir = new Path(System.getProperty("test.build.data",
System.getProperty("java.io.tmpdir", tmpDir.toString()))
}
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.
tmpDir.toString()
thxs, i'll modify it later
HashPartitioner.class.getName()); | ||
conf.setStrings(TezRuntimeFrameworkConfigs.LOCAL_DIRS, workingDir.toString()); | ||
OutputContext outputContext = OutputTestHelpers.createOutputContext(conf, workingDir); | ||
TezCounter mapOutputByteCounter = outputContext.getCounters().findCounter(TaskCounter.OUTPUT_BYTES); |
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.
Could we add some some assertion to verify the result of metrics?
Configuration conf = new Configuration(); | ||
FileSystem localFs = FileSystem.getLocal(conf); | ||
Path workingDir = new Path(System.getProperty("test.build.data", | ||
System.getProperty("java.io.tmpdir", "/tmp")), |
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.
ditto.
HashPartitioner.class.getName()); | ||
conf.setStrings(TezRuntimeFrameworkConfigs.LOCAL_DIRS, workingDir.toString()); | ||
OutputContext outputContext = OutputTestHelpers.createOutputContext(conf, workingDir); | ||
TezCounter mapOutputByteCounter = outputContext.getCounters().findCounter(TaskCounter.OUTPUT_BYTES); |
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.
ditto.
@lifeSo @zhengchenyu Could you help review this pr? |
I found only mapOutputByteCounter was added. Do you plan to add other counters? |
LGTM +1 |
You should run the command to format the files
|
ok |
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.
ok, good!
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, thanks @bin41215 @lifeSo @zhengchenyu , merged to master.
What changes were proposed in this pull request?
Add output mapOutputByteCounter metrics
Why are the changes needed?
add this metrics to fix reducing the number of tasks
Fix: #972
Does this PR introduce any user-facing change?
No.
How was this patch tested?
ut