Details
-
Type:
Sub-task
-
Status: Resolved
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: None
-
Fix Version/s: None
-
Component/s: task
-
Labels:None
Description
Running test with valgrind shows there are some bugs, this jira try to fix them.
Activity
- All
- Comments
- Work Log
- History
- Activity
- Transitions
+ } else { + return RawFileSystemInstance;
This doesn't seem quite right. How about we THROW instead?
Can you add a test for the bug fix in ToString, and the fix in CounterSet?
Hi Binglin, could you explain a bit about "2. use open instead of fopen in FileInputStream/FileOutputStream" ?
I've also run nttest with valgrind and there is a bunch of "blocks still reachable or indirectly lost" in the outputs. Is there a serious bug in fopen ?
update patch, changes:
1. fix more valgrind errors in nttest, run nttest now only show errors in libz and libjvm
2. remove some excessive log in test
3. remove java filesystem and related factory methods (getDefaultFsUri, FileSystem::get)
4. FileInputStream/FileOutputStream use open instead of fopen, this is mostly an optimization.
5. add unittest for CounterSet
6. add unittest for ToHexString
7. fix memory leak in PartitionBucket
1. About toHex
-string StringUtil::ToString(const void * v, uint32_t len) {
+static char ToHex(uint8_t v)Unknown macro: { + return v < 10 ? (v + '0') }
It is not safe to not doing range check for a public function, besides the correct implementation which convert binary to string should use base64 encoding. Since StringUtil::ToString(const void * v, uint32_t len) is only used for md5 conversion,
case MD5HashType:
dest.append(StringUtil::ToString(data, length));
I believe we can rename StringUtil::ToString(const void * v, uint32_t len) to StringUtil::md5BinaryToString(const void * v, uint32_t len), and also make ToHex(uint8_t v) private or inlined to md5BinaryToString.
2. memmov replace memcpy is good, thanks
3. About
} else { // no more, pop heap
+ delete _heap[0];
There is another leak at Merge
MergeEntryPtr * base = &(_heap[0]);
popHeap(base, base + cur_heap_size, _comparator);
_heap.pop_back();
And, I suggest we can add a comments in source about why we delete _heap[0]
Others looks good, +1
Thanks for the review Sean.
I believe we can rename StringUtil::ToString(const void * v, uint32_t len) to StringUtil::md5BinaryToString(const void * v, uint32_t len), and also make ToHex(uint8_t v) private or inlined to md5BinaryToString.
I make the function name from ToString to ToHexString, hex means hex format binary string, which is md5's format, this format is not restricted to md5 only, so I think ToHexString is more suitable. About ToHex, using static prefix makes this method private, thats why I add static.
There is another leak at Merge
What do you mean? I already add delete in the code you specified. Do we need to adding comments only?
Hi Binling,
About the leak, at src / main / native / src / lib / Merge.cc, there is a similar mem leak, you only fixed the leak in PartitionBucketIterator.cc in patch https://issues.apache.org/jira/secure/attachment/12658416/MAPREDUCE-6005.v2.patch
About toHexString, the name is good. However, maybe better use snsprintf(buf_ptr, "%02X", ...)
for (i = 0; i < size; i++)
{ buf_ptr += snsprintf(buf_ptr, "%02X", buf[i]); }Hi Sean,
About the leak, at src / main / native / src / lib / Merge.cc, there is a similar mem leak, you only fixed the leak in PartitionBucketIterator.cc
In merger, all MergeEntryPtr is owned by Merger::_entries, and is deleted in ~Merger at end, so it doesn't require additional care.
maybe better use snsprintf(buf_ptr, "%02X", ...)
Sure, I will update the patch
In merger, all MergeEntryPtr is owned by Merger::_entries, and is deleted in ~Merger at end, so it doesn't require additional care.
you are right, +1.
One more:
Can you also fix
string StringUtil::ToString(int32_t v)
Unknown macro: { char tmp[32]; snprintf(tmp, 32, "%d", v); return tmp; }string StringUtil::ToString(uint32_t v)
Unknown macro: { char tmp[32]; snprintf(tmp, 32, "%u", v); return tmp; }string StringUtil::ToString(int64_t v)
Unknown macro: { char tmp[32]; snprintf(tmp, 32, "%lld", (long long int)v); return tmp; }string StringUtil::ToString(int64_t v, char pad, int64_t len)
Unknown macro: { char tmp[32]; snprintf(tmp, 32, "%%%c%lldlld", pad, len); return Format(tmp, v); }string StringUtil::ToString(uint64_t v)
Unknown macro: { char tmp[32]; snprintf(tmp, 32, "%llu", (long long unsigned int)v); return tmp; }string StringUtil::ToString(bool v) {
if (v)Unknown macro: { return "true"; }else
Unknown macro: { return "false"; }}
string StringUtil::ToString(float v)
Unknown macro: { char tmp[32]; snprintf(tmp, 32, "%f", v); return tmp; }string StringUtil::ToString(double v)
Unknown macro: { char tmp[32]; snprintf(tmp, 32, "%lf", v); return tmp; }
1) it is not safe to convert a char array to a string like this. It will trigger a copy contructor. But by http://www.cplusplus.com/reference/string/string/string/,
string (const char* s);
the string need to be null terminated.
2) snprintf(tmp, 32, "%lf", v) impl is platform dependant when size “32” equals the v length. It may truncate the raw data, or may ignore the null terminitor. http://linux.die.net/man/3/snprintf,
The functions snprintf() and vsnprintf() write at most size bytes (including the terminating null byte ('\0')) to str.
Thanks for the comments Sean. I change toString(double) and toString(float) to use Format, which is safe. For other fixed int types, buffer size 32 should be sufficient.
Thanks. +1
I have committed this, thanks Sean.
SUCCESS: Integrated in Hadoop-Yarn-trunk #679 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/679/)
MAPREDUCE-6005. native-task: Fix some valgrind errors (Binglin Chang) (bchang: rev 83a396733ea43564beee928f41041a341e26db05)
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestComparatorForStdSort.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/PartitionBucketIterator.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestReadBuffer.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/codec/GzipCodec.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/Buffers.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestMemBlockIterator.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestReadWriteBuffer.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/util/StringUtil.h
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/MemoryBlock.h
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/util/WritableUtils.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/TestCommand.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestIterator.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestComparatorForDualPivotQuickSort.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestPartitionBucket.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/TestCounter.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/util/StringUtil.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/NativeObjectFactory.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestFixSizeContainer.cc
- hadoop-mapreduce-project/CHANGES.
MAPREDUCE-2841.txt - hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/TestMain.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/FileSystem.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestByteBuffer.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestMemoryBlock.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestKVBuffer.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/TestFileSystem.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/TestIFile.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/NativeObjectFactory.h
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/util/TestStringUtil.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/FileSystem.h
FAILURE: Integrated in Hadoop-Mapreduce-trunk #1895 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1895/)
MAPREDUCE-6005. native-task: Fix some valgrind errors (Binglin Chang) (bchang: rev 83a396733ea43564beee928f41041a341e26db05)
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/TestCommand.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestReadBuffer.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/TestMain.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestByteBuffer.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestIterator.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/util/WritableUtils.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/FileSystem.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestComparatorForStdSort.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/MemoryBlock.h
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/TestFileSystem.cc
- hadoop-mapreduce-project/CHANGES.
MAPREDUCE-2841.txt - hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/util/StringUtil.h
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/FileSystem.h
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/Buffers.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestMemoryBlock.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/util/TestStringUtil.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/NativeObjectFactory.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestMemBlockIterator.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/PartitionBucketIterator.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/TestCounter.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestFixSizeContainer.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestReadWriteBuffer.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/util/StringUtil.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/TestIFile.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestPartitionBucket.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/codec/GzipCodec.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestComparatorForDualPivotQuickSort.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/NativeObjectFactory.h
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestKVBuffer.cc
SUCCESS: Integrated in Hadoop-Hdfs-trunk #1870 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1870/)
MAPREDUCE-6005. native-task: Fix some valgrind errors (Binglin Chang) (bchang: rev 83a396733ea43564beee928f41041a341e26db05)
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestByteBuffer.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/Buffers.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/PartitionBucketIterator.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/TestCounter.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestReadWriteBuffer.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestComparatorForDualPivotQuickSort.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestComparatorForStdSort.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/TestFileSystem.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/MemoryBlock.h
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestPartitionBucket.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/util/StringUtil.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/TestMain.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestIterator.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestReadBuffer.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/NativeObjectFactory.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/util/StringUtil.h
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/FileSystem.h
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestMemoryBlock.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/codec/GzipCodec.cc
- hadoop-mapreduce-project/CHANGES.
MAPREDUCE-2841.txt - hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/FileSystem.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/TestCommand.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/util/WritableUtils.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestMemBlockIterator.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/util/TestStringUtil.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/TestIFile.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestFixSizeContainer.cc
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/src/lib/NativeObjectFactory.h
- hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-nativetask/src/main/native/test/lib/TestKVBuffer.cc
Changes:
1. Bugfix: should use memmove instead of memcpy in ReadBuffer::fillGet, this probably cause TestGlibCBug fails on ubuntu, after the patch, TestGlibCBug no longer fails.
2. use open instead of fopen in FileInputStream/FileOutputStream
3. Bugfix: set<Counter*> need to use a comparator
4. Bugfix: StringUtil::ToString(const void * v, uint32_t len)
5. Bugfix: object not free in TestIFile.cc
6. Bugfix: object not free in TestMain.cc