[
https://issues.apache.org/jira/browse/MAPREDUCE-6067?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14122344#comment-14122344
]
Sean Zhong commented on MAPREDUCE-6067:
---------------------------------------
Hi Binling,
{quote}
Thanks for the comments Manu and Sean.
Since the new added line lies in the critical path of performance. May be it is
risky to change here?
1. The added code just increase 2 counters, the performance impact should be
negligible, and we need a way to get the counter number right? java side also
increase counters for every kv pair.
{quote}
I am 100% understand what you are doing, counter is important. I also belive
that there should no performance impact. The reason that I raise this out is
that this part of code is tuned for CPU cache efficiency for months, and during
that process, I did saw some seeming trivival minor changes, like one or two
lines of change, will impact the CPU cache efficiency.
I am OK that you commit this change, I just want to remind the potential risk.
{quote}
In common practice, log added when troubleshooting bug should be remove after
the bug is found and fixed.
{quote}
The troubleshooting I mean is operation team troubleshooting for customers.
{quote}
spill file path is useful only for debugging only
{quote}
I think operation team will want this. In field, myself have been benefited
from this line of code during onsite custom support. I wil want to know where
the intermediate files goes.
{quote}
LOG("MemoryPool is full, fail to allocate new MemBlock, block size: %d, kv
length: %d", expect, kvLength);
{quote}
For this line, I agreed that you should remove it.
For the merge log, spill log, I do think we should keep them. First, there are
only a few lines of them, it will not impact the readbility. Second, It
indicate the normal application flow.
It is strange that there is no log for combiner Like this when I enabled the
combiner.
{quote}
- if (total_record != 0) {
- LOG("[Merge] Merged segment#: %lu, record#: %"PRIu64", avg record size:
%"PRIu64", uncompressed total bytes: %"PRIu64", compressed total bytes:
%"PRIu64", time: %"PRIu64" ms",
- _entries.size(),
- total_record,
- output_size / (total_record),
- output_size,
- real_output_size,
- interval / M);
{quote}
{quote}
4. Simple use local var doesn't work, if we want to eliminate field member, we
need a way to get both outputpath and job from sub-methods(runNativeTest,
runNormalTest), perhaps just inline them into test method, this is lot change
compare to current approach, if you think it's OK, I will make more aggressive
changes.
{quote}
I think more changes are better than this. Some test maven plugin allows tests
to be runned in parallel, sharing mutable stuff in Test Cases is wrong.
{quote}
I see you already add cleanUp method to remove root dir, so the old cleanup
code is removed
{quote}
OK.
{quote}
+ Counter * materializedBytes =
NativeObjectFactory::GetCounter(TaskCounters::TASK_COUNTER_GROUP,
+ TaskCounters::MAP_OUTPUT_MATERIALIZED_BYTES);
{quote}
Yes, but you only have a declaration, but never use it?
> native-task: spilled records counter is incorrect
> -------------------------------------------------
>
> Key: MAPREDUCE-6067
> URL: https://issues.apache.org/jira/browse/MAPREDUCE-6067
> Project: Hadoop Map/Reduce
> Issue Type: Sub-task
> Components: task
> Reporter: Todd Lipcon
> Assignee: Binglin Chang
> Attachments: MAPREDUCE-6067.v1.patch, MAPREDUCE-6067.v2.patch,
> MAPREDUCE-6067.v3.patch, MAPREDUCE-6067.v4.patch, native-counters.html,
> trunk-counters.html
>
>
> After running a terasort, I see the spilled records counter at 5028651606,
> which is about half what I expected to see. Using the non-native collector I
> see the expected count of 10000000000. It seems the correct number of records
> were indeed spilled, because the job's output record count is correct.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
