You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "ye yuqiang (Code Review)" <ge...@cloudera.org> on 2019/04/30 06:44:22 UTC
[kudu-CR] replace nvml with memkind in block cache
ye yuqiang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13188
Change subject: replace nvml with memkind in block cache
......................................................................
replace nvml with memkind in block cache
Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
---
M CMakeLists.txt
A cmake_modules/FindMemkind.cmake
D cmake_modules/FindPmem.cmake
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
A thirdparty/install-memkind-from-git.sh
M thirdparty/vars.sh
16 files changed, 238 insertions(+), 233 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/13188/1
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 1
Gerrit-Owner: ye yuqiang <yu...@intel.com>
[kudu-CR] replace nvml with memkind in block cache
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind in block cache
......................................................................
Patch Set 1:
It looks like we'd need to add libnuma to thirdparty as well for this to build.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 1
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Apr 2019 16:00:22 +0000
Gerrit-HasComments: No
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13188
to look at the new patch set (#7).
Change subject: replace nvml with memkind
......................................................................
replace nvml with memkind
The current nvm cache based on PMDK, formerly know as NVML. For volatile
memory usage(libvmem), PMDK will not have a long-term maintenance. Since
persistent memory support has been integrated into libmemkind, memkind is
the recommended choice for any new volatile usages, since it combines
support for multiple types of volatile memory into a single, convenient API.
https://pmem.io/pmdk/
Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
---
M CMakeLists.txt
A cmake_modules/FindMemkind.cmake
A cmake_modules/FindNuma.cmake
D cmake_modules/FindPmem.cmake
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
16 files changed, 291 insertions(+), 224 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/13188/7
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 7
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] replace nvml with memkind in block cache
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind in block cache
......................................................................
Patch Set 1:
> Let me know if you have any issues; happy to help fix.
We also just added a section to CONTRIBUTING.md about adding new third-party dependencies which you might find useful. It's not yet available on the website but if you sync with master you can see it in docs/CONTRIBUTING.md.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 1
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Wed, 08 May 2019 19:45:05 +0000
Gerrit-HasComments: No
[kudu-CR] replace nvml with memkind in block cache
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
ye yuqiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind in block cache
......................................................................
Patch Set 1:
> I don't have a lot of context or knowledge of these libraries so I
> have a few high level questions.
>
> 1. What is the motivation for this change? Is there an expected
> performance or stability improvement?
>
> 2. Why did you change to memkind from nvml? Note: the nvml library
> looks like it changed names/repos (https://github.com/pmem/pmdk)
Yes, PMDK(old name is NVML) is a comprehensive library for persistent memory. For volatile memory usage(libvmem), PMDK will not have a long-term maintenance, it has been integrated into memkind. https://pmem.io/pmdk/.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 1
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Mon, 06 May 2019 02:43:52 +0000
Gerrit-HasComments: No
[kudu-CR] replace nvml with memkind
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 14:
(8 comments)
Looks great. Just a few nits remaining.
http://gerrit.cloudera.org:8080/#/c/13188/14//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13188/14//COMMIT_MSG@7
PS14, Line 7: replace nvml with memkind
Please prefix this line with the JIRA:
KUDU-2605: replace nvml with memkind
http://gerrit.cloudera.org:8080/#/c/13188/14/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13188/14/CMakeLists.txt@449
PS14, Line 449:
Please revert this changed line.
http://gerrit.cloudera.org:8080/#/c/13188/14/cmake_modules/FindMemkind.cmake
File cmake_modules/FindMemkind.cmake:
http://gerrit.cloudera.org:8080/#/c/13188/14/cmake_modules/FindMemkind.cmake@18
PS14, Line 18: Required
required
http://gerrit.cloudera.org:8080/#/c/13188/14/cmake_modules/FindNuma.cmake
File cmake_modules/FindNuma.cmake:
http://gerrit.cloudera.org:8080/#/c/13188/14/cmake_modules/FindNuma.cmake@18
PS14, Line 18: Required
required
http://gerrit.cloudera.org:8080/#/c/13188/14/src/kudu/util/cache-test.cc
File src/kudu/util/cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13188/14/src/kudu/util/cache-test.cc@498
PS14, Line 498: : CacheBaseTest(16 * 1024 * 1024) {
Why was the cache size changed from 14M to 16M?
http://gerrit.cloudera.org:8080/#/c/13188/14/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/13188/14/src/kudu/util/nvm_cache.cc@113
PS14, Line 113:
Please revert this line change.
http://gerrit.cloudera.org:8080/#/c/13188/14/src/kudu/util/nvm_cache.cc@612
PS14, Line 612: handle->kv_data
Are you sure this change makes sense? The base of the allocation is ph.get() (or handle, or buf). handle->kv_data is an address sizeof(LRUHandle) bytes beyond the allocation base.
http://gerrit.cloudera.org:8080/#/c/13188/14/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/13188/14/thirdparty/build-definitions.sh@801
PS14, Line 801: LDFLAGS="$EXTRA_LDFLAGS -L$PREFIX/lib" \
Does this also need -Wl,-rpath=$PREFIX/lib?
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 14
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Fri, 24 May 2019 18:17:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2605: replace nvml with memkind
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: KUDU-2605: replace nvml with memkind
......................................................................
KUDU-2605: replace nvml with memkind
The current nvm cache based on PMDK, formerly know as NVML. For volatile
memory usage(libvmem), PMDK will not have a long-term maintenance. Since
persistent memory support has been integrated into libmemkind, memkind is
the recommended choice for any new volatile usages, since it combines
support for multiple types of volatile memory into a single, convenient API.
Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Reviewed-on: http://gerrit.cloudera.org:8080/13188
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
M CMakeLists.txt
A cmake_modules/FindMemkind.cmake
A cmake_modules/FindNuma.cmake
D cmake_modules/FindPmem.cmake
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
16 files changed, 317 insertions(+), 224 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 16
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
ye yuqiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 9:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13188/9/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/13188/9/thirdparty/build-definitions.sh@754
PS9, Line 754: $NUMACTL_SOURCE/configure \
> Could we feed EXTRA_CFLAGS, EXTRA_CXXFLAGS (if numactl is C++), LDFLAGS, an
numactl can not pass the compile with tsan. Will have some issue like undefined reference to `__tsan_init' for some numa benchmark code, but no option to separate them from source code when we build numactl. Since it only a dependency for memkind, is it a MUST for us to add these flags?
http://gerrit.cloudera.org:8080/#/c/13188/9/thirdparty/build-definitions.sh@777
PS9, Line 777: CFLAGS="$EXTRA_CFLAGS -O3" \
> thirdparty/build-thirdparty.sh sets -O2 in EXTRA_CXXFLAGS. Perhaps we shoul
EXTRA_CXXFLAGS does not work for memkind. It's strange since I can find this FLAGS in memkind configure help. So I have to add -O3 in CFLAGS (will change to O2 if it's better)
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 9
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Tue, 21 May 2019 03:38:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13188
to look at the new patch set (#5).
Change subject: replace nvml with memkind
......................................................................
replace nvml with memkind
The current nvm cache based on PMDK, formerly know as NVML. For volatile
memory usage(libvmem), PMDK will not have a long-term maintenance. Since
persistent memory support has been integrated into libmemkind, memkind is
the recommended choice for any new volatile usages, since it combines
support for multiple types of volatile memory into a single, convenient API.
https://pmem.io/pmdk/
Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
---
M CMakeLists.txt
A cmake_modules/FindMemkind.cmake
A cmake_modules/FindNuma.cmake
D cmake_modules/FindPmem.cmake
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
16 files changed, 291 insertions(+), 224 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/13188/5
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 5
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] KUDU-2605: replace nvml with memkind
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: KUDU-2605: replace nvml with memkind
......................................................................
Patch Set 15: Verified+1 Code-Review+2
Overriding Jenkins, failures were known flakes unrelated to this patch.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 15
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Sun, 26 May 2019 18:12:28 +0000
Gerrit-HasComments: No
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13188
to look at the new patch set (#8).
Change subject: replace nvml with memkind
......................................................................
replace nvml with memkind
The current nvm cache based on PMDK, formerly know as NVML. For volatile
memory usage(libvmem), PMDK will not have a long-term maintenance. Since
persistent memory support has been integrated into libmemkind, memkind is
the recommended choice for any new volatile usages, since it combines
support for multiple types of volatile memory into a single, convenient API.
https://pmem.io/pmdk/
Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
---
M CMakeLists.txt
A cmake_modules/FindMemkind.cmake
A cmake_modules/FindNuma.cmake
D cmake_modules/FindPmem.cmake
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
16 files changed, 295 insertions(+), 224 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/13188/8
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 8
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] replace nvml with memkind in block cache
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind in block cache
......................................................................
Patch Set 1:
I don't have a lot of context or knowledge of these libraries so I have a few high level questions.
1. What is the motivation for this change? Is there an expected performance or stability improvement?
2. Why did you change to memkind from nvml? Note: the nvml library looks like it changed names/repos (https://github.com/pmem/pmdk)
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 1
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Apr 2019 16:04:14 +0000
Gerrit-HasComments: No
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13188
to look at the new patch set (#10).
Change subject: replace nvml with memkind
......................................................................
replace nvml with memkind
The current nvm cache based on PMDK, formerly know as NVML. For volatile
memory usage(libvmem), PMDK will not have a long-term maintenance. Since
persistent memory support has been integrated into libmemkind, memkind is
the recommended choice for any new volatile usages, since it combines
support for multiple types of volatile memory into a single, convenient API.
Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
---
M CMakeLists.txt
A cmake_modules/FindMemkind.cmake
A cmake_modules/FindNuma.cmake
D cmake_modules/FindPmem.cmake
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/client/symbols.map
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
17 files changed, 320 insertions(+), 224 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/13188/10
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 10
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] KUDU-2605: replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
ye yuqiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: KUDU-2605: replace nvml with memkind
......................................................................
Patch Set 15:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13188/14/src/kudu/util/cache-test.cc
File src/kudu/util/cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13188/14/src/kudu/util/cache-test.cc@498
PS14, Line 498: : CacheBaseTest(16 * 1024 * 1024) {
> Why was the cache size changed from 14M to 16M?
This relate to nvm_cache.cc Line 640. 14M is the minimum size required by nvml to initialize while 16M is for memkind.
http://gerrit.cloudera.org:8080/#/c/13188/14/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/13188/14/src/kudu/util/nvm_cache.cc@612
PS14, Line 612:
> Are you sure this change makes sense? The base of the allocation is ph.get(
Fix. revert it to buf. I was doing some another code change. But there's no need to change here.
http://gerrit.cloudera.org:8080/#/c/13188/14/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/13188/14/thirdparty/build-definitions.sh@801
PS14, Line 801: LDFLAGS="$EXTRA_LDFLAGS -L$PREFIX/lib -Wl,-rpath=$PREFIX/lib" \
> Does this also need -Wl,-rpath=$PREFIX/lib?
Fix. Thanks for your review.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 15
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Sat, 25 May 2019 12:57:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind in block cache
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind in block cache
......................................................................
Patch Set 1:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt@162
PS1, Line 162: add_definitions(-DJE_PREFIX=jemk_)
Why is this necessary? Please add a comment.
http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt@452
PS1, Line 452: # disable ld.gold due some compilation issue for numa
Given that NOT NUMA_LIB_PATH on L1167 yields a fatal error, this effectively disables gold universally. That's not acceptable; gold provides a significant link time speedup for Kudu's large C++ binaries. Can you please get to the bottom of this issue and fix it?
http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt@1166
PS1, Line 1166: find_library(NUMA_LIB_PATH numa)
Like Todd asked, why is libnuma required in the system vs. added as a thirdparty dependency? How did you reach that decision?
http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt@1171
PS1, Line 1171: SHARED_LIB "${NUMA_LIB_PATH}")
With only a shared library, does this mean the exported C++ client library (i.e. lib/exported/libkudu_client.so) now depends on libnuma.so in the target system? That's probably not OK, unless it's a dependency that we expect to find universally.
http://gerrit.cloudera.org:8080/#/c/13188/1/cmake_modules/FindMemkind.cmake
File cmake_modules/FindMemkind.cmake:
http://gerrit.cloudera.org:8080/#/c/13188/1/cmake_modules/FindMemkind.cmake@21
PS1, Line 21: # XXX_STATIC_LIBS, path to *.a
: # XXX_SHARED_LIBS, path to *.so shared library
Nit: replace the XXX with MEMKIND.
Also, the generated variables end with _LIB, not _LIBS.
http://gerrit.cloudera.org:8080/#/c/13188/1/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/13188/1/src/kudu/util/nvm_cache.cc@a229
PS1, Line 229:
This comment is still correct, except that it wraps memkind_malloc now.
http://gerrit.cloudera.org:8080/#/c/13188/1/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/13188/1/thirdparty/build-definitions.sh@753
PS1, Line 753: mkdir -p $PREFIX/include/jemalloc
: mkdir -p $PREFIX/include/memkind/internal
Not necessary if you use rsync in the installation steps, which may be preferable as it'll mean that installation does the right thing vis a vis old files if the list of files shrinks.
http://gerrit.cloudera.org:8080/#/c/13188/1/thirdparty/build-definitions.sh@757
PS1, Line 757: # It doesn't appear possible to isolate source and build directories, so just
: # prepopulate the latter using the former.
Is this still true for memkind?
http://gerrit.cloudera.org:8080/#/c/13188/1/thirdparty/build-definitions.sh@763
PS1, Line 763: # manually install the built artifacts.
The memkind build script doesn't offer a "make install" equivalent?
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 1
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Apr 2019 22:48:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13188/12/src/kudu/client/symbols.map
File src/kudu/client/symbols.map:
http://gerrit.cloudera.org:8080/#/c/13188/12/src/kudu/client/symbols.map@53
PS12, Line 53: # jemalloc
: operator*;
> The bad symbol comes from https://github.com/jemalloc/jemalloc/blob/dev/src
Right. That seems to be a new addition in jemalloc.
Could we pass --disable-cxx into the jemalloc build using $EXTRA_CONF? According to https://github.com/jemalloc/jemalloc/blob/dev/INSTALL.md, that'll exclude jemalloc_cpp.cpp from the build which is probably fine for memkind, as it is written in C and doesn't need C++ support.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 12
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Fri, 24 May 2019 07:25:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13188/4/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13188/4/CMakeLists.txt@1154
PS4, Line 1154: DEPS "${MEMKIND_DEPS}")
You can just add numa here directly; no need to indirect via the MEMKIND_DEPS variable (since it's not used anywhere else).
http://gerrit.cloudera.org:8080/#/c/13188/4/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/13188/4/src/kudu/util/cache.h@33
PS4, Line 33: DECLARE_bool(cache_force_single_shard);
You shouldn't need to do this in a header file; can you declare it in nvm_cache.cc directly?
http://gerrit.cloudera.org:8080/#/c/13188/2/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/13188/2/thirdparty/build-definitions.sh@767
PS2, Line 767: rsync -av --delete $MEMKIND_SOURCE/ .
> memkind 1.9.0 is not support -fsanitize=thread -fno-omit-frame-pointer whil
If you can identify the particular upstream patch (or patches) that fixed this, we can add them to our distribution. See the thirdparty/patches/ subdirectory and the patch application process in thirdparty/download-thirdparty.sh.
Looking into it in more detail, here's what I think is going on:
1. I don't think -fsanitize=thread is the issue; I think the problem is building memkind with clang from the thirdparty tree. For some reason that prevents /usr/include/omp.h from being included. That's arguably a good thing; we don't necessarily want the Kudu build to depend on yet another library that may not be found locally.
2. AFAICT, the omp.h dependency is only in memkind tests. I think we're building them by virtue of calling build.sh and running `make all`. Perhaps if we copied the parts of build.sh into this shell script instead of invoking build.sh, we could avoid that issue altogether?
3. I also think the JE_PREFIX macro need only be set when building memkind. It doesn't need to be set when building Kudu itself.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 4
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Fri, 17 May 2019 18:07:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
ye yuqiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 9:
> The TSAN version of client_symbol-test failed with:
>
> Found bad symbol 'operator delete[](void*)'
> Found bad symbol 'operator delete[](void*, std::nothrow_t const&)'
> Found bad symbol 'operator delete(void*)'
> Found bad symbol 'operator delete(void*, std::nothrow_t const&)'
> Found bad symbol 'operator new[](unsigned long)'
> Found bad symbol 'operator new[](unsigned long, std::nothrow_t
> const&)'
> Found bad symbol 'operator new(unsigned long)'
> Found bad symbol 'operator new(unsigned long, std::nothrow_t
> const&)'
>
> These are probably jemalloc symbols, but it's strange that it only
> manifested in the TSAN build.
TSAN and RELEASE should have some logic for jemalloc compiling since it's a dependency for memkind. I did not find these symbol in jemalloc or memkind, but they are in llvm tsan_new_delete.cc
Also, there's another unit test failure. http://dist-test.cloudera.org/job?job_id=jenkins-slave.1558363209.16531
1) testBasicMasterOperations(org.apache.kudu.client.TestAuthTokenReacquire)
java.lang.AssertionError: test failed: unexpected errors
at org.junit.Assert.fail(Assert.java:88)
at org.apache.kudu.client.TestAuthTokenReacquire.testBasicMasterOperations(TestAuthTokenReacquire.java:153)
I am still not sure how the patch will affect this unit test and other 5 unit test in DEBUG
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 9
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Tue, 21 May 2019 03:16:08 +0000
Gerrit-HasComments: No
[kudu-CR] replace nvml with memkind
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 12:
(2 comments)
> > Build Failed
> >
> > http://jenkins.kudu.apache.org/job/kudu-gerrit/17613/ : FAILURE
>
> Looks like there still will be one unit test failure. Is this also not related to this patch?
It's not related, you can ignore it.
http://gerrit.cloudera.org:8080/#/c/13188/9/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13188/9/CMakeLists.txt@1165
PS9, Line 1165: endif()
Why don't we need this anymore? Is it because we expressed the memkind->numa dependency on L1150?
http://gerrit.cloudera.org:8080/#/c/13188/12/src/kudu/client/symbols.map
File src/kudu/client/symbols.map:
http://gerrit.cloudera.org:8080/#/c/13188/12/src/kudu/client/symbols.map@53
PS12, Line 53: # jemalloc
: operator*;
This means that we can potentially ship a Kudu client library with an embedded malloc implementation. We have client APIs that allocate memory from inside the client library, then hand it over to the application to use and deallocate (and vice versa). If there are two malloc implementations at play (one in the library and one in the application), allocation in one and deallocation in the other could lead to memory corruption.
The older nvml library also bundled jemalloc, and this wasn't an issue then. Why is it an issue with memkind? What are we doing differently?
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 12
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Fri, 24 May 2019 05:30:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
ye yuqiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13188/12/src/kudu/client/symbols.map
File src/kudu/client/symbols.map:
http://gerrit.cloudera.org:8080/#/c/13188/12/src/kudu/client/symbols.map@53
PS12, Line 53: # jemalloc
: operator*;
> This means that we can potentially ship a Kudu client library with an embed
The bad symbol comes from https://github.com/jemalloc/jemalloc/blob/dev/src/jemalloc_cpp.cpp.
Looks like the jemalloc in nvml is very old version. I can even not find this cpp file.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 12
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Fri, 24 May 2019 05:44:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
ye yuqiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 2:
> (9 comments)
Numactl and Memkind are downloading from S3 bucket now.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 2
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Tue, 14 May 2019 06:22:23 +0000
Gerrit-HasComments: No
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13188
to look at the new patch set (#11).
Change subject: replace nvml with memkind
......................................................................
replace nvml with memkind
The current nvm cache based on PMDK, formerly know as NVML. For volatile
memory usage(libvmem), PMDK will not have a long-term maintenance. Since
persistent memory support has been integrated into libmemkind, memkind is
the recommended choice for any new volatile usages, since it combines
support for multiple types of volatile memory into a single, convenient API.
Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
---
M CMakeLists.txt
A cmake_modules/FindMemkind.cmake
A cmake_modules/FindNuma.cmake
D cmake_modules/FindPmem.cmake
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/client/symbols.map
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
17 files changed, 322 insertions(+), 224 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/13188/11
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 11
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] replace nvml with memkind in block cache
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
ye yuqiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind in block cache
......................................................................
Patch Set 1:
> It looks like we'd need to add libnuma to thirdparty as well for
> this to build.
Will add libnuma to thirdparty, libnuma is required by memkind. Downloading from github is not very reliable, but currently I have to do it. These libraries is not in cloudfront.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 1
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Mon, 06 May 2019 02:53:18 +0000
Gerrit-HasComments: No
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13188
to look at the new patch set (#3).
Change subject: replace nvml with memkind
......................................................................
replace nvml with memkind
Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
---
M CMakeLists.txt
A cmake_modules/FindMemkind.cmake
A cmake_modules/FindNuma.cmake
D cmake_modules/FindPmem.cmake
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
16 files changed, 271 insertions(+), 236 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/13188/3
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 3
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
ye yuqiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 12:
> Build Failed
>
> http://jenkins.kudu.apache.org/job/kudu-gerrit/17613/ : FAILURE
Looks like there still will be one unit test failure. Is this also not related to this patch?
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 12
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Fri, 24 May 2019 01:52:44 +0000
Gerrit-HasComments: No
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
ye yuqiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt@1166
PS1, Line 1166: SHARED_LIB "${NUMA_SHARED_LIB}")
> Could you answer this question, possibly also in the commit message?
libnuma is required by memkind library. I've added it as a thirdparty library.
http://gerrit.cloudera.org:8080/#/c/13188/2/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13188/2/CMakeLists.txt@1165
PS2, Line 1165: # STATIC_LIB "${NUMA_STATIC_LIB}"
> Why is this commented out?
During the compilation, some library will depends on libnuma.so instead of linuma.a, like file-cache-test. So I only export a shared library. This line will be deleted if it's ok
http://gerrit.cloudera.org:8080/#/c/13188/1/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/13188/1/src/kudu/util/nvm_cache.cc@54
PS1, Line 54: DEFINE_bool(nvm_cache_force_single_shard, false,
: "Override all cache implementations to use just one shard");
: TAG_FLAG(nvm_cache_force_single_shard, hidden);
:
> According to the comment, this is targeted for tests only. If so, why not
This is not for tests only. I add this flag to test case as 'cache_force_single_shard'
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 3
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Wed, 15 May 2019 00:47:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 9:
(2 comments)
> Also, there's another unit test failure. http://dist-test.cloudera.org/job?job_id=jenkins-slave.1558363209.16531
>
> 1) testBasicMasterOperations(org.apache.kudu.client.TestAuthTokenReacquire)
> java.lang.AssertionError: test failed: unexpected errors
> at org.junit.Assert.fail(Assert.java:88)
> at org.apache.kudu.client.TestAuthTokenReacquire.testBasicMasterOperations(TestAuthTokenReacquire.java:153)
>
> I am still not sure how the patch will affect this unit test and other 5 unit test in DEBUG
This failure is unrelated to your patch and you can ignore it.
http://gerrit.cloudera.org:8080/#/c/13188/9/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/13188/9/thirdparty/build-definitions.sh@754
PS9, Line 754: $NUMACTL_SOURCE/configure \
> numactl can not pass the compile with tsan. Will have some issue like undef
I tested this locally, and I was able to both repro the build error and fix it:
Failed: CC=/home/adar/Source/kudu/thirdparty/clang-toolchain/bin/clang CFLAGS=-fsanitize=thread LDFLAGS="-L/home/adar/Source/kudu/thirdparty/installed/tsan/lib -Wl,-rpath=/home/adar/Source/kudu/thirdparty/installed/tsan/lib" ./configure && make
Passed: CC=/home/adar/Source/kudu/thirdparty/clang-toolchain/bin/clang CFLAGS=-fsanitize=thread LDFLAGS="-fsanitize=thread -L/home/adar/Source/kudu/thirdparty/installed/tsan/lib -Wl,-rpath=/home/adar/Source/kudu/thirdparty/installed/tsan/lib" ./configure && make
It seems like it's necessary to add -fsanitize=thread to LDFLAGS too. You can do that locally, or you could try to add it to EXTRA_LDFLAGS globally.
http://gerrit.cloudera.org:8080/#/c/13188/9/thirdparty/build-definitions.sh@777
PS9, Line 777: CFLAGS="$EXTRA_CFLAGS -O3" \
> EXTRA_CXXFLAGS does not work for memkind. It's strange since I can find thi
Yeah, memkind will ignore CXXFLAGS because it's not a C++ library. But hopefully CFLAGS will do the trick.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 9
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Wed, 22 May 2019 19:03:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
ye yuqiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13188/2/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/13188/2/thirdparty/build-definitions.sh@767
PS2, Line 767: rsync -av --delete $MEMKIND_SOURCE/ .
> If you can identify the particular upstream patch (or patches) that fixed t
thanks for your advice. yes, missing omp.h is caused by building with clang. But -fsantize=thread does not work for both gcc and clang here. We may find out the patch from memkind upstream.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 4
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Sat, 18 May 2019 00:57:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13188
to look at the new patch set (#2).
Change subject: replace nvml with memkind
......................................................................
replace nvml with memkind
Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
---
M CMakeLists.txt
A cmake_modules/FindMemkind.cmake
A cmake_modules/FindNuma.cmake
D cmake_modules/FindPmem.cmake
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
16 files changed, 269 insertions(+), 234 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/13188/2
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 2
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] replace nvml with memkind in block cache
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind in block cache
......................................................................
Patch Set 1:
> Will add libnuma to thirdparty, libnuma is required by memkind. Downloading from github is not very reliable, but currently I have to do it. These libraries is not in cloudfront.
I uploaded numactl-2.0.12.tar.gz and memkind-1.9.0.tar.gz to the S3 bucket. They should be available via CloudFront in the usual thirdparty location, so you can remove the github-based install logic. Let me know if you have any issues; happy to help fix.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 1
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Tue, 07 May 2019 04:31:22 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2605: replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13188
to look at the new patch set (#15).
Change subject: KUDU-2605: replace nvml with memkind
......................................................................
KUDU-2605: replace nvml with memkind
The current nvm cache based on PMDK, formerly know as NVML. For volatile
memory usage(libvmem), PMDK will not have a long-term maintenance. Since
persistent memory support has been integrated into libmemkind, memkind is
the recommended choice for any new volatile usages, since it combines
support for multiple types of volatile memory into a single, convenient API.
Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
---
M CMakeLists.txt
A cmake_modules/FindMemkind.cmake
A cmake_modules/FindNuma.cmake
D cmake_modules/FindPmem.cmake
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
16 files changed, 317 insertions(+), 224 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/13188/15
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 15
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
ye yuqiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 9:
> > The TSAN version of client_symbol-test failed with:
> >
> > Found bad symbol 'operator delete[](void*)'
> > Found bad symbol 'operator delete[](void*, std::nothrow_t
> const&)'
> > Found bad symbol 'operator delete(void*)'
> > Found bad symbol 'operator delete(void*, std::nothrow_t const&)'
> > Found bad symbol 'operator new[](unsigned long)'
> > Found bad symbol 'operator new[](unsigned long, std::nothrow_t
> > const&)'
> > Found bad symbol 'operator new(unsigned long)'
> > Found bad symbol 'operator new(unsigned long, std::nothrow_t
> > const&)'
> >
> > These are probably jemalloc symbols, but it's strange that it
> only
> > manifested in the TSAN build.
>
> TSAN and RELEASE should have some logic for jemalloc compiling
> since it's a dependency for memkind. I did not find these symbol in
> jemalloc or memkind, but they are in llvm tsan_new_delete.cc
> Also, there's another unit test failure. http://dist-test.cloudera.org/job?job_id=jenkins-slave.1558363209.16531
>
> 1) testBasicMasterOperations(org.apache.kudu.client.TestAuthTokenReacquire)
> java.lang.AssertionError: test failed: unexpected errors
> at org.junit.Assert.fail(Assert.java:88)
> at org.apache.kudu.client.TestAuthTokenReacquire.testBasicMasterOperations(TestAuthTokenReacquire.java:153)
>
> I am still not sure how the patch will affect this unit test and
> other 5 unit test in DEBUG
Oh. the bad symbols comes from jemalloc. Looks like we also need copy the code out from build_jemalloc.sh
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 9
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Tue, 21 May 2019 09:07:14 +0000
Gerrit-HasComments: No
[kudu-CR] replace nvml with memkind
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 9:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13188/9/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/13188/9/thirdparty/build-definitions.sh@754
PS9, Line 754: $NUMACTL_SOURCE/configure \
Could we feed EXTRA_CFLAGS, EXTRA_CXXFLAGS (if numactl is C++), LDFLAGS, and LIBS into this?
http://gerrit.cloudera.org:8080/#/c/13188/9/thirdparty/build-definitions.sh@765
PS9, Line 765: # it doesn't appear possible to isolate source and build directories, so just
Nit: should start with capital I here.
http://gerrit.cloudera.org:8080/#/c/13188/9/thirdparty/build-definitions.sh@769
PS9, Line 769: if [ -z "$JE_PREFIX" ]; then
: export JE_PREFIX=jemk_
: fi
Two things:
1. You shouldn't need to check if JE_PREFIX has already been defined; no other script in the thirdparty build defines it.
2. It would be nice to unexport it when the memkind build is finished.
http://gerrit.cloudera.org:8080/#/c/13188/9/thirdparty/build-definitions.sh@772
PS9, Line 772:
There should be a comment here explaining that all of this stuff was derived from memkind's build.sh script, which we can't run directly because we want to avoid building the tests.
http://gerrit.cloudera.org:8080/#/c/13188/9/thirdparty/build-definitions.sh@777
PS9, Line 777: CFLAGS="$EXTRA_CFLAGS -O3" \
thirdparty/build-thirdparty.sh sets -O2 in EXTRA_CXXFLAGS. Perhaps we should set also set it in EXTRA_CFLAGS, so we can avoid this override? Also, is there a noticeable improvement from -O3 to -O2? In the past we observed that -O2 can sometimes result in better performance (see commit 1f6f818f1).
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 9
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Mon, 20 May 2019 18:28:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13188
to look at the new patch set (#6).
Change subject: replace nvml with memkind
......................................................................
replace nvml with memkind
The current nvm cache based on PMDK, formerly know as NVML. For volatile
memory usage(libvmem), PMDK will not have a long-term maintenance. Since
persistent memory support has been integrated into libmemkind, memkind is
the recommended choice for any new volatile usages, since it combines
support for multiple types of volatile memory into a single, convenient API.
https://pmem.io/pmdk/
Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
---
M CMakeLists.txt
A cmake_modules/FindMemkind.cmake
A cmake_modules/FindNuma.cmake
D cmake_modules/FindPmem.cmake
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
16 files changed, 291 insertions(+), 224 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/13188/6
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 6
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13188
to look at the new patch set (#14).
Change subject: replace nvml with memkind
......................................................................
replace nvml with memkind
The current nvm cache based on PMDK, formerly know as NVML. For volatile
memory usage(libvmem), PMDK will not have a long-term maintenance. Since
persistent memory support has been integrated into libmemkind, memkind is
the recommended choice for any new volatile usages, since it combines
support for multiple types of volatile memory into a single, convenient API.
Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
---
M CMakeLists.txt
A cmake_modules/FindMemkind.cmake
A cmake_modules/FindNuma.cmake
D cmake_modules/FindPmem.cmake
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
16 files changed, 319 insertions(+), 224 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/13188/14
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 14
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
ye yuqiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13188/1/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/13188/1/src/kudu/util/nvm_cache.cc@54
PS1, Line 54: "The path at which the NVM cache will try to allocate its memory. "
: "This can be a tmpfs or ramfs for testing purposes.");
: TAG_FLAG(nvm_cache_path, experimental);
:
> Not sure I understand. The comment on L54 (and the 'hidden' tag) suggest th
Declare cache_force_single_shard in cache.h for reuse
http://gerrit.cloudera.org:8080/#/c/13188/2/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/13188/2/thirdparty/build-definitions.sh@765
PS2, Line 765: # It doesn't appear possible to isolate source and build directories, so just
> Could you keep the "It doesn't appear possible..." comment so it's clear wh
Added
http://gerrit.cloudera.org:8080/#/c/13188/2/thirdparty/build-definitions.sh@767
PS2, Line 767: rsync -av --delete $MEMKIND_SOURCE/ .
> Should we feed in EXTRA_CFLAGS, EXTRA_LDFLAGS, and EXTRA_LIBS here?
memkind 1.9.0 is not support -fsanitize=thread -fno-omit-frame-pointer while it works for latest memkind. Any suggestions here?
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 4
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Fri, 17 May 2019 08:03:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13188
to look at the new patch set (#13).
Change subject: replace nvml with memkind
......................................................................
replace nvml with memkind
The current nvm cache based on PMDK, formerly know as NVML. For volatile
memory usage(libvmem), PMDK will not have a long-term maintenance. Since
persistent memory support has been integrated into libmemkind, memkind is
the recommended choice for any new volatile usages, since it combines
support for multiple types of volatile memory into a single, convenient API.
Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
---
M CMakeLists.txt
A cmake_modules/FindMemkind.cmake
A cmake_modules/FindNuma.cmake
D cmake_modules/FindPmem.cmake
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
16 files changed, 318 insertions(+), 224 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/13188/13
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 13
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Grant Henke, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13188
to look at the new patch set (#4).
Change subject: replace nvml with memkind
......................................................................
replace nvml with memkind
The current nvm cache based on PMDK, formerly know as NVML. For volatile
memory usage(libvmem), PMDK will not have a long-term maintenance. Since
persistent memory support has been integrated into libmemkind, memkind is
the recommended choice for any new volatile usages, since it combines
support for multiple types of volatile memory into a single, convenient API.
https://pmem.io/pmdk/
Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
---
M CMakeLists.txt
A cmake_modules/FindMemkind.cmake
A cmake_modules/FindNuma.cmake
D cmake_modules/FindPmem.cmake
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/cfile-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M thirdparty/LICENSE.txt
M thirdparty/build-definitions.sh
M thirdparty/build-thirdparty.sh
M thirdparty/download-thirdparty.sh
M thirdparty/vars.sh
16 files changed, 261 insertions(+), 205 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/13188/4
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 4
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] replace nvml with memkind
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 2:
(9 comments)
Looks like there are still some test failures to work out.
> > I don't have a lot of context or knowledge of these libraries so I
> > have a few high level questions.
> >
> > 1. What is the motivation for this change? Is there an expected
> > performance or stability improvement?
> >
> > 2. Why did you change to memkind from nvml? Note: the nvml library
> > looks like it changed names/repos (https://github.com/pmem/pmdk)
>
> Yes, PMDK(old name is NVML) is a comprehensive library for persistent memory. For volatile memory usage(libvmem), PMDK will not have a long-term maintenance, it has been integrated into memkind. https://pmem.io/pmdk/.
Could you incorporate this information into your commit message too?
http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt@1166
PS1, Line 1166: SHARED_LIB "${NUMA_SHARED_LIB}")
> Like Todd asked, why is libnuma required in the system vs. added as a third
Could you answer this question, possibly also in the commit message?
http://gerrit.cloudera.org:8080/#/c/13188/2/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13188/2/CMakeLists.txt@1165
PS2, Line 1165: # STATIC_LIB "${NUMA_STATIC_LIB}"
Why is this commented out?
http://gerrit.cloudera.org:8080/#/c/13188/1/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/13188/1/src/kudu/util/nvm_cache.cc@54
PS1, Line 54: DEFINE_bool(nvm_cache_force_single_shard, false,
: "Override all cache implementations to use just one shard");
: TAG_FLAG(nvm_cache_force_single_shard, hidden);
:
> According to the comment, this is targeted for tests only. If so, why not
Could you address Alexey's question?
http://gerrit.cloudera.org:8080/#/c/13188/2/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/13188/2/src/kudu/util/nvm_cache.cc@652
PS2, Line 652:
Got some leading whitespace here.
http://gerrit.cloudera.org:8080/#/c/13188/2/thirdparty/LICENSE.txt
File thirdparty/LICENSE.txt:
http://gerrit.cloudera.org:8080/#/c/13188/2/thirdparty/LICENSE.txt@542
PS2, Line 542: thirdparty/nvml-*/
Don't we need to preserve the exception for jemalloc? Looking upstream, https://github.com/memkind/memkind/blob/master/jemalloc/COPYING still has the same copyright notices as it did when bundled in nvml.
http://gerrit.cloudera.org:8080/#/c/13188/2/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
http://gerrit.cloudera.org:8080/#/c/13188/2/thirdparty/build-definitions.sh@765
PS2, Line 765: rsync -av --delete $MEMKIND_SOURCE/ .
Could you keep the "It doesn't appear possible..." comment so it's clear why this is happening?
http://gerrit.cloudera.org:8080/#/c/13188/2/thirdparty/build-definitions.sh@767
PS2, Line 767: $MEMKIND_BDIR/build.sh LDFLAGS="-L$PREFIX/lib" CPPFLAGS="-I$PREFIX/include" --prefix=$PREFIX
Should we feed in EXTRA_CFLAGS, EXTRA_LDFLAGS, and EXTRA_LIBS here?
http://gerrit.cloudera.org:8080/#/c/13188/2/thirdparty/build-definitions.sh@769
PS2, Line 769: head
header
http://gerrit.cloudera.org:8080/#/c/13188/2/thirdparty/vars.sh
File thirdparty/vars.sh:
http://gerrit.cloudera.org:8080/#/c/13188/2/thirdparty/vars.sh@169
PS2, Line 169: # numactl 2.0.12 is required build memkind library.
Nit: "numactl is required to build memkind library".
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 2
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Tue, 14 May 2019 23:06:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 9:
The TSAN version of client_symbol-test failed with:
Found bad symbol 'operator delete[](void*)'
Found bad symbol 'operator delete[](void*, std::nothrow_t const&)'
Found bad symbol 'operator delete(void*)'
Found bad symbol 'operator delete(void*, std::nothrow_t const&)'
Found bad symbol 'operator new[](unsigned long)'
Found bad symbol 'operator new[](unsigned long, std::nothrow_t const&)'
Found bad symbol 'operator new(unsigned long)'
Found bad symbol 'operator new(unsigned long, std::nothrow_t const&)'
These are probably jemalloc symbols, but it's strange that it only manifested in the TSAN build.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 9
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Mon, 20 May 2019 18:09:30 +0000
Gerrit-HasComments: No
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
ye yuqiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 2:
(3 comments)
> (2 comments)
http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt@162
PS1, Line 162: # compiler flags for different build types (run 'cmake -DCMAKE_BUILD_TYPE=<type> .')
> Why is this necessary? Please add a comment.
This variable is required by memkind head file. Moved it to memkind part
http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt@452
PS1, Line 452: return()
> Given that NOT NUMA_LIB_PATH on L1167 yields a fatal error, this effectivel
Fixed
http://gerrit.cloudera.org:8080/#/c/13188/1/CMakeLists.txt@1171
PS1, Line 1171: find_package(CURL REQUIRED)
> With only a shared library, does this mean the exported C++ client library
During the compilation, some library will depends on libnuma.so instead of linuma.a, like file-cache-test. So I export a shared library.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 2
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Tue, 14 May 2019 06:37:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind in block cache
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind in block cache
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13188/1/thirdparty/install-memkind-from-git.sh
File thirdparty/install-memkind-from-git.sh:
PS1:
> This is a new style of installing third-party dependencies compared to usin
I think we used to do more direct github downloads but found that github was slower and less reliable than cloudfront, so we changed over to duplicating artifacts into our S3 bucket
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 1
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Apr 2019 17:00:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind in block cache
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind in block cache
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13188/1/thirdparty/install-memkind-from-git.sh
File thirdparty/install-memkind-from-git.sh:
PS1:
This is a new style of installing third-party dependencies compared to using the s3 bucket. I don't mind it, but perhaps we should use the bucket to be consistent. We can wait to see what others think.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 1
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Apr 2019 16:05:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind in block cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind in block cache
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13188/1/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/13188/1/src/kudu/util/nvm_cache.cc@54
PS1, Line 54: // Useful in tests that require accurate cache capacity accounting.
: DEFINE_bool(nvm_cache_force_single_shard, false,
: "Override all cache implementations to use just one shard");
: TAG_FLAG(nvm_cache_force_single_shard, hidden);
According to the comment, this is targeted for tests only. If so, why not to use already existing 'cache_force_single_shard' flag then?
http://gerrit.cloudera.org:8080/#/c/13188/1/thirdparty/vars.sh
File thirdparty/vars.sh:
http://gerrit.cloudera.org:8080/#/c/13188/1/thirdparty/vars.sh@169
PS1, Line 169: MEMKIND_VERSION=1.7.0
2019-04-02 memkind v1.9.0 was released. Does it make sense to use the latest version instead of 1.7.0?
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 1
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 02 May 2019 18:54:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13188/2/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13188/2/CMakeLists.txt@1165
PS2, Line 1165: # STATIC_LIB "${NUMA_STATIC_LIB}"
> During the compilation, some library will depends on libnuma.so instead of
No, we absolutely need to have a static library for our RELEASE builds, so that the Kudu binaries have no external dependencies. There are some exceptions, but only for commonly installed libraries with good ABI compatibility, like openssl and libstdc++. I don't think libnuma meets that criteria.
http://gerrit.cloudera.org:8080/#/c/13188/1/src/kudu/util/nvm_cache.cc
File src/kudu/util/nvm_cache.cc:
http://gerrit.cloudera.org:8080/#/c/13188/1/src/kudu/util/nvm_cache.cc@54
PS1, Line 54: DEFINE_bool(nvm_cache_force_single_shard, false,
: "Override all cache implementations to use just one shard");
: TAG_FLAG(nvm_cache_force_single_shard, hidden);
:
> This is not for tests only. I add this flag to test case as 'cache_force_si
Not sure I understand. The comment on L54 (and the 'hidden' tag) suggest that the gflag is only for testing. Are you saying it's also for production? If so, why?
What Alexey is asking is if you can reuse the existing cache_force_single_shard flag instead of defining a new one. Would that be possible?
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 3
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Wed, 15 May 2019 00:58:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
ye yuqiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13188/9/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/13188/9/CMakeLists.txt@1165
PS9, Line 1165: endif()
> Why don't we need this anymore? Is it because we expressed the memkind->num
Yes, it's only a dependency of memkind. No need add it to KUDU_BASE_LIBS
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 12
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Fri, 24 May 2019 05:35:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2605: replace nvml with memkind
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed a vote on this change.
Change subject: KUDU-2605: replace nvml with memkind
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 15
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
[kudu-CR] replace nvml with memkind in block cache
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
ye yuqiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind in block cache
......................................................................
Patch Set 1:
> > Let me know if you have any issues; happy to help fix.
>
> We also just added a section to CONTRIBUTING.md about adding new
> third-party dependencies which you might find useful. It's not yet
> available on the website but if you sync with master you can see it
> in docs/CONTRIBUTING.md.
thanks for your reply. That's really helpful.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 1
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Thu, 09 May 2019 01:50:57 +0000
Gerrit-HasComments: No
[kudu-CR] replace nvml with memkind
Posted by "ye yuqiang (Code Review)" <ge...@cloudera.org>.
ye yuqiang has posted comments on this change. ( http://gerrit.cloudera.org:8080/13188 )
Change subject: replace nvml with memkind
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13188/12/src/kudu/client/symbols.map
File src/kudu/client/symbols.map:
http://gerrit.cloudera.org:8080/#/c/13188/12/src/kudu/client/symbols.map@53
PS12, Line 53: };
:
> Right. That seems to be a new addition in jemalloc.
Thanks for your advice.Will try it.
--
To view, visit http://gerrit.cloudera.org:8080/13188
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08855c62bc42b9e6f7a4b3bfa27a708ed2c133f5
Gerrit-Change-Number: 13188
Gerrit-PatchSet: 13
Gerrit-Owner: ye yuqiang <yu...@intel.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: ye yuqiang <yu...@intel.com>
Gerrit-Comment-Date: Fri, 24 May 2019 08:29:51 +0000
Gerrit-HasComments: Yes