[Impala-ASF-CR] IMPALA-4758: (2/2) Impala-side changes to build with latest gutil
Henry Robinson has submitted this change and it was merged. Change subject: IMPALA-4758: (2/2) Impala-side changes to build with latest gutil .. IMPALA-4758: (2/2) Impala-side changes to build with latest gutil Meant to be taken as a whole with the previous commit. This patch makes the necessary code changes to Impala and the gutil/ library to fix all compilation errors. Future upgrades to gutil/ should redo the work in this commit. * Remove kudu/ include prefix with command: git grep -l "include \"kudu/" | xargs sed -i 's/include \"kudu\//include \"/g' * Change KUDU_GUTIL_* guards to be GUTIL_* git grep -l KUDU_GUTIL | xargs sed -i 's/KUDU_GUTIL/GUTIL/g' * Replace glog/logging.h with common/logging.h git grep -l "glog/logging" | xargs sed -i 's/glog\/logging/common\/logging/g' * Provide our own implementation of since-removed MonotonicNanos() * Reinstate COMPILE_FLAGS argument to ADD_EXPORTABLE_LIBRARY, used by gutil. * Replay overwritten parts of following commits: a7c3f30 - Remove AMD Opteron Rev E workaround from atomicops 54194af - IMPALA-4631: don't use floating point operations for time unit conversions 152c586 - Improve AtomicInt abstraction and implementation * Comment out non-compiling deprecated function definitions in numbers.h * Overwrite changes from 92fafa "Use more efficient gutil implementation of Log2Ceiling" in favour of implementing them in Impala code only. * Couple of misc fixes. Change-Id: I4ac21d7d6401f21fcdfdd1132b8f322bfba4bb80 Reviewed-on: http://gerrit.cloudera.org:8080/5688 Reviewed-by: Dan Hecht Tested-by: Impala Public Jenkins --- M be/src/exec/hash-table.inline.h M be/src/gutil/atomic_refcount.h M be/src/gutil/atomicops-internals-x86.cc M be/src/gutil/atomicops-internals-x86.h M be/src/gutil/atomicops.h M be/src/gutil/auxiliary/atomicops-internals-arm-generic.h M be/src/gutil/auxiliary/atomicops-internals-arm-v6plus.h M be/src/gutil/auxiliary/atomicops-internals-windows.h M be/src/gutil/basictypes.h M be/src/gutil/bind.h M be/src/gutil/bind_helpers.h M be/src/gutil/bind_internal.h M be/src/gutil/bits.cc M be/src/gutil/bits.h M be/src/gutil/callback.h M be/src/gutil/callback_forward.h M be/src/gutil/callback_internal.cc M be/src/gutil/callback_internal.h M be/src/gutil/casts.h M be/src/gutil/charmap.h M be/src/gutil/cpu.cc M be/src/gutil/cycleclock-inl.h M be/src/gutil/endian.h M be/src/gutil/fixedarray.h M be/src/gutil/gscoped_ptr.h M be/src/gutil/hash/builtin_type_hash.h M be/src/gutil/hash/city.cc M be/src/gutil/hash/city.h M be/src/gutil/hash/hash.cc M be/src/gutil/hash/hash.h M be/src/gutil/hash/hash128to64.h M be/src/gutil/hash/jenkins.cc M be/src/gutil/hash/jenkins.h M be/src/gutil/hash/jenkins_lookup2.h M be/src/gutil/hash/legacy_hash.h M be/src/gutil/hash/string_hash.h M be/src/gutil/int128.cc M be/src/gutil/int128.h M be/src/gutil/macros.h M be/src/gutil/manual_constructor.h M be/src/gutil/map-util.h M be/src/gutil/mathlimits.cc M be/src/gutil/once.cc M be/src/gutil/once.h M be/src/gutil/paranoid.h M be/src/gutil/port.h M be/src/gutil/raw_scoped_refptr_mismatch_checker.h M be/src/gutil/ref_counted.cc M be/src/gutil/ref_counted.h M be/src/gutil/ref_counted_memory.cc M be/src/gutil/ref_counted_memory.h M be/src/gutil/singleton.h M be/src/gutil/spinlock.cc M be/src/gutil/spinlock.h M be/src/gutil/spinlock_internal.cc M be/src/gutil/spinlock_internal.h M be/src/gutil/spinlock_linux-inl.h M be/src/gutil/stl_util.h M be/src/gutil/stringprintf.cc M be/src/gutil/stringprintf.h M be/src/gutil/strings/ascii_ctype.cc M be/src/gutil/strings/charset.cc M be/src/gutil/strings/charset.h M be/src/gutil/strings/escaping.cc M be/src/gutil/strings/escaping.h M be/src/gutil/strings/fastmem.h M be/src/gutil/strings/human_readable.cc M be/src/gutil/strings/human_readable.h M be/src/gutil/strings/join.cc M be/src/gutil/strings/join.h M be/src/gutil/strings/memutil.cc M be/src/gutil/strings/memutil.h M be/src/gutil/strings/numbers.cc M be/src/gutil/strings/numbers.h M be/src/gutil/strings/serialize.cc M be/src/gutil/strings/serialize.h M be/src/gutil/strings/split.cc M be/src/gutil/strings/split.h M be/src/gutil/strings/split_internal.h M be/src/gutil/strings/strcat.cc M be/src/gutil/strings/strcat.h M be/src/gutil/strings/string_util-test.cc M be/src/gutil/strings/stringpiece.cc M be/src/gutil/strings/stringpiece.h M be/src/gutil/strings/strip.cc M be/src/gutil/strings/strip.h M be/src/gutil/strings/substitute.cc M be/src/gutil/strings/substitute.h M be/src/gutil/strings/util.cc M be/src/gutil/strings/util.h M be/src/gutil/strtoint.cc M be/src/gutil/strtoint.h M be/src/gutil/synchronization_profiling.h M be/src/gutil/sysinfo.cc M be/src/gutil/threading/thread_collision_warner.cc M be/src/gutil/threading/thread_collision_warner.h M be/src/gutil/type_traits.h M be/src/gutil/walltime.cc M be/src/gutil/walltime.h M be/src/runtime/buffered-tuple-stream-test.cc M be/src/runtime/buffered-tuple-stream-v2-test.
[Impala-ASF-CR] IMPALA-4758: (1/2) Update gutil/ from Kudu@a1bfd7b
Henry Robinson has posted comments on this change. Change subject: IMPALA-4758: (1/2) Update gutil/ from Kudu@a1bfd7b .. Patch Set 8: Verified+1 Validated as part of this GVO: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/420/ Note that https://gerrit.cloudera.org/#/c/5688/ is required for this change to compile, so I'm submitting them together. -- To view, visit http://gerrit.cloudera.org:8080/5687 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic708a9c4e76ede17af9b06e0a0a8e9ae7d357960 Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4758: (1/2) Update gutil/ from Kudu@a1bfd7b
Henry Robinson has submitted this change and it was merged. Change subject: IMPALA-4758: (1/2) Update gutil/ from Kudu@a1bfd7b .. IMPALA-4758: (1/2) Update gutil/ from Kudu@a1bfd7b * Copy gutil from Kudu * Minimal changes to gutil/CMakeLists.txt Change-Id: Ic708a9c4e76ede17af9b06e0a0a8e9ae7d357960 Reviewed-on: http://gerrit.cloudera.org:8080/5687 Reviewed-by: Dan Hecht Tested-by: Henry Robinson --- M be/src/gutil/CMakeLists.txt D be/src/gutil/algorithm.h M be/src/gutil/atomic_refcount.h M be/src/gutil/atomicops-internals-powerpc.h A be/src/gutil/atomicops-internals-tsan.h M be/src/gutil/atomicops-internals-x86.cc M be/src/gutil/atomicops-internals-x86.h M be/src/gutil/atomicops.h M be/src/gutil/auxiliary/atomicops-internals-arm-generic.h M be/src/gutil/auxiliary/atomicops-internals-arm-v6plus.h D be/src/gutil/auxiliary/atomicops-internals-macosx.h M be/src/gutil/auxiliary/atomicops-internals-windows.h M be/src/gutil/basictypes.h A be/src/gutil/bind.h A be/src/gutil/bind.h.pump A be/src/gutil/bind_helpers.h A be/src/gutil/bind_internal.h A be/src/gutil/bind_internal.h.pump M be/src/gutil/bits.cc M be/src/gutil/bits.h A be/src/gutil/callback.h A be/src/gutil/callback.h.pump A be/src/gutil/callback_forward.h A be/src/gutil/callback_internal.cc A be/src/gutil/callback_internal.h M be/src/gutil/casts.h M be/src/gutil/charmap.h A be/src/gutil/cpu.cc A be/src/gutil/cpu.h M be/src/gutil/cycleclock-inl.h R be/src/gutil/dynamic_annotations.c M be/src/gutil/dynamic_annotations.h M be/src/gutil/endian.h M be/src/gutil/fixedarray.h M be/src/gutil/gscoped_ptr.h M be/src/gutil/hash/builtin_type_hash.h M be/src/gutil/hash/city.cc M be/src/gutil/hash/city.h M be/src/gutil/hash/hash.cc M be/src/gutil/hash/hash.h M be/src/gutil/hash/hash128to64.h M be/src/gutil/hash/jenkins.cc M be/src/gutil/hash/jenkins.h M be/src/gutil/hash/jenkins_lookup2.h M be/src/gutil/hash/legacy_hash.h M be/src/gutil/hash/string_hash.h M be/src/gutil/int128.cc M be/src/gutil/int128.h M be/src/gutil/integral_types.h M be/src/gutil/linux_syscall_support.h M be/src/gutil/logging-inl.h M be/src/gutil/macros.h M be/src/gutil/manual_constructor.h M be/src/gutil/map-util.h M be/src/gutil/mathlimits.cc M be/src/gutil/mathlimits.h M be/src/gutil/move.h M be/src/gutil/once.cc M be/src/gutil/once.h M be/src/gutil/paranoid.h M be/src/gutil/port.h D be/src/gutil/proto/types.pb.cc D be/src/gutil/proto/types.pb.h D be/src/gutil/proto/types.proto A be/src/gutil/raw_scoped_refptr_mismatch_checker.h A be/src/gutil/ref_counted.cc A be/src/gutil/ref_counted.h A be/src/gutil/ref_counted_memory.cc A be/src/gutil/ref_counted_memory.h M be/src/gutil/singleton.h D be/src/gutil/sparsetable.h M be/src/gutil/spinlock.cc M be/src/gutil/spinlock.h M be/src/gutil/spinlock_internal.cc M be/src/gutil/spinlock_internal.h M be/src/gutil/spinlock_linux-inl.h M be/src/gutil/spinlock_win32-inl.h M be/src/gutil/stl_util.h M be/src/gutil/stringprintf.cc M be/src/gutil/stringprintf.h M be/src/gutil/strings/ascii_ctype.cc M be/src/gutil/strings/charset.cc M be/src/gutil/strings/charset.h M be/src/gutil/strings/escaping.cc M be/src/gutil/strings/escaping.h M be/src/gutil/strings/fastmem.h M be/src/gutil/strings/human_readable.cc M be/src/gutil/strings/human_readable.h M be/src/gutil/strings/join.cc M be/src/gutil/strings/join.h M be/src/gutil/strings/memutil.cc M be/src/gutil/strings/memutil.h M be/src/gutil/strings/numbers.cc M be/src/gutil/strings/numbers.h M be/src/gutil/strings/serialize.cc M be/src/gutil/strings/serialize.h M be/src/gutil/strings/split.cc M be/src/gutil/strings/split.h M be/src/gutil/strings/split_internal.h M be/src/gutil/strings/strcat.cc M be/src/gutil/strings/strcat.h A be/src/gutil/strings/string_util-test.cc M be/src/gutil/strings/stringpiece.cc M be/src/gutil/strings/stringpiece.h M be/src/gutil/strings/strip.cc M be/src/gutil/strings/strip.h M be/src/gutil/strings/substitute.cc M be/src/gutil/strings/substitute.h M be/src/gutil/strings/util.cc M be/src/gutil/strings/util.h M be/src/gutil/strtoint.cc M be/src/gutil/strtoint.h M be/src/gutil/synchronization_profiling.h M be/src/gutil/sysinfo.cc M be/src/gutil/sysinfo.h M be/src/gutil/template_util.h A be/src/gutil/threading/thread_collision_warner.cc A be/src/gutil/threading/thread_collision_warner.h M be/src/gutil/type_traits.h R be/src/gutil/utf/rune.c M be/src/gutil/walltime.cc M be/src/gutil/walltime.h 122 files changed, 9,196 insertions(+), 4,102 deletions(-) Approvals: Henry Robinson: Verified Dan Hecht: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/5687 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ic708a9c4e76ede17af9b06e0a0a8e9ae7d357960 Gerrit-PatchSet: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4758: (2/2) Impala-side changes to build with latest gutil
Todd Lipcon has posted comments on this change. Change subject: IMPALA-4758: (2/2) Impala-side changes to build with latest gutil .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/5688/9//COMMIT_MSG Commit Message: Line 32: a7c3f30 - Remove AMD Opteron Rev E workaround from atomicops probably worth our taking this patch into kudu gutil as well -- To view, visit http://gerrit.cloudera.org:8080/5688 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4ac21d7d6401f21fcdfdd1132b8f322bfba4bb80 Gerrit-PatchSet: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[Impala-ASF-CR] Fix missing declaration of std::max in hdfs-avro-scanner-test
Henry Robinson has submitted this change and it was merged. Change subject: Fix missing declaration of std::max in hdfs-avro-scanner-test .. Fix missing declaration of std::max in hdfs-avro-scanner-test Change-Id: Id41c14e98163bcbb03675969e0a53754e92a4c14 Reviewed-on: http://gerrit.cloudera.org:8080/6497 Reviewed-by: Sailesh Mukil Tested-by: Henry Robinson --- M be/src/exec/hdfs-avro-scanner-test.cc 1 file changed, 3 insertions(+), 0 deletions(-) Approvals: Henry Robinson: Verified Sailesh Mukil: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/6497 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Id41c14e98163bcbb03675969e0a53754e92a4c14 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] Fix missing declaration of std::max in hdfs-avro-scanner-test
Henry Robinson has posted comments on this change. Change subject: Fix missing declaration of std::max in hdfs-avro-scanner-test .. Patch Set 1: Verified+1 Validated as part of this GVO: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/420/ -- To view, visit http://gerrit.cloudera.org:8080/6497 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id41c14e98163bcbb03675969e0a53754e92a4c14 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
Henry Robinson has posted comments on this change. Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting() .. Patch Set 1: Code-Review+1 (5 comments) http://gerrit.cloudera.org:8080/#/c/6502/1/be/src/common/atomic.h File be/src/common/atomic.h: Line 122: public: indentation http://gerrit.cloudera.org:8080/#/c/6502/1/be/src/runtime/mem-tracker.cc File be/src/runtime/mem-tracker.cc: Line 123: ReservationTrackerCounters* new_counters = new ReservationTrackerCounters; is there an compiler-generated copy-c'tor for ReservationTrackerCounters? PS1, Line 198: delete it's really tempting to have the AtomicPtr delete its contents on destruction, but I guess we should make a separate AtomicScopedPtr if we have more use cases for it. PS1, Line 307: gc_functions_[i](); if these are caller-supplied, should the interface say something about how they should be non-blocking, since they're executed while holding a spinlock? http://gerrit.cloudera.org:8080/#/c/6502/1/be/src/runtime/mem-tracker.h File be/src/runtime/mem-tracker.h: PS1, Line 386: Must be read and written atomically I'd remove this. Or say "may be read concurrently". It's not clear how a reader could arrange to read this atomically (unless it CAS-es the pointer out of the AtomicPtr?), and I'm not sure that's what the comment means to say. -- To view, visit http://gerrit.cloudera.org:8080/6502 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Henry Robinson Gerrit-HasComments: Yes
[Impala-ASF-CR] Fix missing declaration of std::max in hdfs-avro-scanner-test
Sailesh Mukil has posted comments on this change. Change subject: Fix missing declaration of std::max in hdfs-avro-scanner-test .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6497 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id41c14e98163bcbb03675969e0a53754e92a4c14 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Sailesh Mukil Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4758: (2/2) Impala-side changes to build with latest gutil
Impala Public Jenkins has posted comments on this change. Change subject: IMPALA-4758: (2/2) Impala-side changes to build with latest gutil .. Patch Set 9: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/5688 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4ac21d7d6401f21fcdfdd1132b8f322bfba4bb80 Gerrit-PatchSet: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4883: Implement Codegen for Union
Taras Bobrovytsky has uploaded a new patch set (#4). Change subject: IMPALA-4883: Implement Codegen for Union .. IMPALA-4883: Implement Codegen for Union For each non-passthrough child of the Union node, codegen Tuple::MarializeExprs(). Testing: Ran test_queries.py test locally in exchaustive mode. Ran some hand crafted performance benchmark queries locally. There is an up to 50% performance improvement. Change-Id: Ib4107d27582ff5416172810364a6e76d3d93c439 --- M be/src/codegen/impala-ir.cc M be/src/exec/CMakeLists.txt A be/src/exec/union-node-ir.cc M be/src/exec/union-node.cc M be/src/exec/union-node.h 5 files changed, 161 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/6459/4 -- To view, visit http://gerrit.cloudera.org:8080/6459 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib4107d27582ff5416172810364a6e76d3d93c439 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5132: Fix ASAN use after free in timezone db
Jim Apple has posted comments on this change. Change subject: IMPALA-5132: Fix ASAN use after free in timezone_db .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/6503/1/be/src/exprs/timezone_db.cc File be/src/exprs/timezone_db.cc: PS1, Line 676: char *filestr = strdup(path.string().c_str()); > Aside: I think in this one case we're OK: C++11 and on, c_str() has to retu I am not suggesting that in this case, though. vector seems like the most understandable solution. -- To view, visit http://gerrit.cloudera.org:8080/6503 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I490f741403ea2004bc51394aa1251577337b1e1d Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Lars Volker Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5132: Fix ASAN use after free in timezone db
Jim Apple has posted comments on this change. Change subject: IMPALA-5132: Fix ASAN use after free in timezone_db .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/6503/1/be/src/exprs/timezone_db.cc File be/src/exprs/timezone_db.cc: PS1, Line 676: char *filestr = strdup(path.string().c_str()); > Thanks, makes sense now. I was unsure about const_cast's casting away behav Aside: I think in this one case we're OK: C++11 and on, c_str() has to return the same value as data(), and data has to return &(*this)[0], which is a char * if this is a string *. -- To view, visit http://gerrit.cloudera.org:8080/6503 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I490f741403ea2004bc51394aa1251577337b1e1d Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Lars Volker Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5132: Fix ASAN use after free in timezone db
Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-5132: Fix ASAN use after free in timezone_db .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/6503/1/be/src/exprs/timezone_db.cc File be/src/exprs/timezone_db.cc: PS1, Line 676: char *filestr = strdup(path.string().c_str()); > No, that's precisely the point: const_cast'ing away a const is undefined in Thanks, makes sense now. I was unsure about const_cast's casting away behavior. -- To view, visit http://gerrit.cloudera.org:8080/6503 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I490f741403ea2004bc51394aa1251577337b1e1d Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4883: Implement Codegen for Union
Taras Bobrovytsky has uploaded a new patch set (#3). Change subject: IMPALA-4883: Implement Codegen for Union .. IMPALA-4883: Implement Codegen for Union For each non-passthrough child of the Union node, codegen Tuple::MarializeExprs(). Testing: Ran test_queries.py test locally in exchaustive mode. Ran some hand crafted performance benchmark queries locally. There is an up to 50% performance improvement. Change-Id: Ib4107d27582ff5416172810364a6e76d3d93c439 --- M be/src/codegen/impala-ir.cc M be/src/exec/CMakeLists.txt A be/src/exec/union-node-ir.cc M be/src/exec/union-node.cc M be/src/exec/union-node.h 5 files changed, 163 insertions(+), 58 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/6459/3 -- To view, visit http://gerrit.cloudera.org:8080/6459 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib4107d27582ff5416172810364a6e76d3d93c439 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Taras Bobrovytsky Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5132: Fix ASAN use after free in timezone db
Henry Robinson has posted comments on this change. Change subject: IMPALA-5132: Fix ASAN use after free in timezone_db .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/6503/1/be/src/exprs/timezone_db.cc File be/src/exprs/timezone_db.cc: PS1, Line 676: char *filestr = strdup(path.string().c_str()); > Oh ya, const_cast too :) No, that's precisely the point: const_cast'ing away a const is undefined in C++. Here, we copy into a representation that has a non-const pointer to the underlying array, so that mkstemp() can mutate it in place. -- To view, visit http://gerrit.cloudera.org:8080/6503 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I490f741403ea2004bc51394aa1251577337b1e1d Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-4166: Add SORT BY sql clause
..
Patch Set 2:
(20 comments)
Initial pass, haven't looked at the tests yet.
http://gerrit.cloudera.org:8080/#/c/6495/2/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:
PS2, Line 986: // SORT BY columns are stored in the 'sort.by.columns' table
property.
: HashMap properties = new HashMap();
: String columnString = Joiner.on(",").join(col_names);
:
properties.put(TablePropertyAnalyzer.TBL_PROP_SORT_BY_COLUMNS, columnString);
: RESULT = new AlterTableSetTblProperties(table, null,
TTablePropertyType.TBL_PROPERTY,
: properties);
: :}
I don't see why you need to treat it as another SetTblProperties statement. I
understand that eventually the sort by columns will be stored as table
properties but this adds unnecessary complexity to the parser. I would simply
create a new AlterTableSetSortByColumns statement.
PS2, Line 1143: opt_sort_by_cols:sort_by_cols
: KW_LIKE table_name:other_table
: opt_comment_val:comment
: file_format_create_table_val:file_format location_val:location
nit: indentation
PS2, Line 1182: opt_sort_by_cols:sort_by_cols opt_comment_val:comment
row_format_val:row_format serde_properties:serde_props
nit: long line
http://gerrit.cloudera.org:8080/#/c/6495/2/fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java
File
fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java:
PS2, Line 162: /**
:* Analyzes the 'sort.by.columns' property to make sure the
columns inside the property
:* occur in the target table and are neither partitioning nor
primary key columns.
:*/
: public void analyzeSortByColumns() throws AnalysisException {
: StringBuilder error = new StringBuilder();
: TablePropertyAnalyzer.analyzeSortByColumns(tblProperties_,
getTargetTable(), error);
: if (error.length() > 0) throw new
AnalysisException(error.toString());
: }
Along the lines of my previous comment: a) I would move this completely out of
this class, b) create a new AlterTableSetSortByColumns and move the analysis
logic there.
http://gerrit.cloudera.org:8080/#/c/6495/2/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:
PS2, Line 275: Boolean
boolean? why class?
PS2, Line 2486: Preconditions.checkState(!globalState_.warningsRetrieved);
This is kind of harsh :). Do we guarantee somewhere that no one will call
addWarning after warnings have been retrieved?
http://gerrit.cloudera.org:8080/#/c/6495/2/fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java
File fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java:
PS2, Line 119: sortByColumns_.size() > 0
!sortByColumns_.isEmpty()
PS2, Line 179: StringBuilder error = new StringBuilder();
: TablePropertyAnalyzer.analyzeSortByColumns(sortByColumns_,
srcTable, error);
: if (error.length() > 0) throw new
AnalysisException(error.toString());
Why don't you just throw the AnalysisException from the analyzeSortByColumns()?
http://gerrit.cloudera.org:8080/#/c/6495/2/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java:
PS2, Line 382: sortByExprs_.size() > 0
!sortByExprs_.isEmpty()
PS2, Line 510: sortByExprs_.add(resultExprs_.get(colIdx));
Does this work ok if you have column permutation?
PS2, Line 768: StringBuilder error = new StringBuilder();
: sortByColumns_ =
TablePropertyAnalyzer.analyzeSortByColumns(table_, error);
: if (error.length() > 0) throw new
AnalysisException(error.toString());
Same comment as before. Move the throw clause inside the analyzeSortByColumns()
function.
http://gerrit.cloudera.org:8080/#/c/6495/2/fe/src/main/java/org/apache/impala/analysis/TablePropertyAnalyzer.java
File fe/src/main/java/org/apache/impala/analysis/TablePropertyAnalyzer.java:
PS2, Line 33: /**
: * This class collects various methods to analyze table
properties.
: */
: public class TablePropertyAnalyzer {
You're only analyzing the sort by columns, so why this generic class? Can't we
move these methods to the TableDef class?
PS2, Line 41: Analyzes the 'sort.by.columns' property of 'table'.
What is the return value?
PS2, Line 86: List partitionCols, List primaryKeyCols
You can simply pass one list which includes the list of column names to exclude
from the sort columns. If you want a specific error message you can construct
it in function analyzeSortByC
[Impala-ASF-CR] IMPALA-5132: Fix ASAN use after free in timezone db
Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-5132: Fix ASAN use after free in timezone_db .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/6503/1/be/src/exprs/timezone_db.cc File be/src/exprs/timezone_db.cc: PS1, Line 676: char *filestr = strdup(path.string().c_str()); > c_str() returns a const char*. Oh ya, const_cast too :) -- To view, visit http://gerrit.cloudera.org:8080/6503 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I490f741403ea2004bc51394aa1251577337b1e1d Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5132: Fix ASAN use after free in timezone db
Henry Robinson has posted comments on this change. Change subject: IMPALA-5132: Fix ASAN use after free in timezone_db .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/6503/1/be/src/exprs/timezone_db.cc File be/src/exprs/timezone_db.cc: PS1, Line 676: char *filestr = strdup(path.string().c_str()); > Or may be just use the string() copy c'tor and copy the string and use path c_str() returns a const char*. -- To view, visit http://gerrit.cloudera.org:8080/6503 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I490f741403ea2004bc51394aa1251577337b1e1d Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5132: Fix ASAN use after free in timezone db
Bharath Vissapragada has posted comments on this change. Change subject: IMPALA-5132: Fix ASAN use after free in timezone_db .. Patch Set 1: (1 comment) I'm fine with Zach's patch too. I was just waiting for the ASAN to pass before pushing it for review. It is a simple patch. http://gerrit.cloudera.org:8080/#/c/6503/1/be/src/exprs/timezone_db.cc File be/src/exprs/timezone_db.cc: PS1, Line 676: char *filestr = strdup(path.string().c_str()); > given the leak(s) below, how about something managed by the stack: Or may be just use the string() copy c'tor and copy the string and use path.c_str()/data() in rest of the code. string path = string(boost::filesystem::string()); mkstemp(filestr.c_str()); -- To view, visit http://gerrit.cloudera.org:8080/6503 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I490f741403ea2004bc51394aa1251577337b1e1d Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5132: Fix ASAN use after free in timezone db
Lars Volker has posted comments on this change. Change subject: IMPALA-5132: Fix ASAN use after free in timezone_db .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/6503/1/be/src/exprs/timezone_db.cc File be/src/exprs/timezone_db.cc: PS1, Line 676: char *filestr = strdup(path.string().c_str()); > given the leak(s) below, how about something managed by the stack: Oh, I wasn't aware it would change the string. Today I learned! :) -- To view, visit http://gerrit.cloudera.org:8080/6503 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I490f741403ea2004bc51394aa1251577337b1e1d Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4041: Limit catalog and admission control updates to coordinators
Impala Public Jenkins has posted comments on this change. Change subject: IMPALA-4041: Limit catalog and admission control updates to coordinators .. Patch Set 6: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/6344 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5f2c74abdbcd60ac050efa323616bd41182ceff3 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4041: Limit catalog and admission control updates to coordinators
Impala Public Jenkins has submitted this change and it was merged. Change subject: IMPALA-4041: Limit catalog and admission control updates to coordinators .. IMPALA-4041: Limit catalog and admission control updates to coordinators With this commit we add the ability to limit catalog updates to a limited set of coordinator nodes. A new startup option, termed 'is_coordinator' is added to indicate if a node is a coordinator. Coordinators accept connections through HS2 and Beeswax interfaces and can also participate in query execution. Non-coordinator nodes do not receive catalog updates from the statestore, do not initialize a query scheduler and cannot accept Beeswax and HS2 client connections. Testing: - Added a custom cluster test that launches a cluster in which the number of coordinators is less than the cluster size and runs a number of smoke queries. - Successfully run exhaustive tests. Change-Id: I5f2c74abdbcd60ac050efa323616bd41182ceff3 Reviewed-on: http://gerrit.cloudera.org:8080/6344 Reviewed-by: Dimitris Tsirogiannis Tested-by: Impala Public Jenkins --- M be/src/runtime/exec-env.cc M be/src/runtime/exec-env.h M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M be/src/scheduling/scheduler-test-util.cc M be/src/scheduling/scheduler-test.cc M be/src/scheduling/scheduler.cc M be/src/scheduling/scheduler.h M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/service/impalad-main.cc M be/src/service/query-exec-state.cc M be/src/testutil/in-process-servers.h M be/src/util/webserver.cc M bin/start-impala-cluster.py M tests/common/custom_cluster_test_suite.py M tests/common/impala_service.py A tests/custom_cluster/test_coordinators.py M www/root.tmpl 19 files changed, 435 insertions(+), 235 deletions(-) Approvals: Impala Public Jenkins: Verified Dimitris Tsirogiannis: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/6344 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I5f2c74abdbcd60ac050efa323616bd41182ceff3 Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs
[Impala-ASF-CR] IMPALA-5132: Fix ASAN use after free in timezone db
Lars Volker has posted comments on this change. Change subject: IMPALA-5132: Fix ASAN use after free in timezone_db .. Patch Set 1: (1 comment) I left some comments, but I understood from the JIRA that Bharath already has a change that he's currently testing in a private ASAN run on Jenkins. Bharath, do you want to push your change for public review? http://gerrit.cloudera.org:8080/#/c/6503/1/be/src/exprs/timezone_db.cc File be/src/exprs/timezone_db.cc: Line 676: char *filestr = strdup(path.string().c_str()); I think you don't need the strdup(), it should be sufficient to keep "path" around as long as filestr is accessed. A nit: we usually put the * next to the type: char* foo -- To view, visit http://gerrit.cloudera.org:8080/6503 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I490f741403ea2004bc51394aa1251577337b1e1d Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5132: Fix ASAN use after free in timezone db
Henry Robinson has posted comments on this change. Change subject: IMPALA-5132: Fix ASAN use after free in timezone_db .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/6503/1/be/src/exprs/timezone_db.cc File be/src/exprs/timezone_db.cc: PS1, Line 676: char *filestr = strdup(path.string().c_str()); given the leak(s) below, how about something managed by the stack: string path = boost::filesystem::string(); vector filestr(path.c_str(), path.c_str() + path.length() + 1); mkstemp(filestr.data()); http://man7.org/linux/man-pages/man3/mkstemp.3.html suggests that mkstemp *will* edit the string in place. PS1, Line 687: return this would leak filestr. -- To view, visit http://gerrit.cloudera.org:8080/6503 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I490f741403ea2004bc51394aa1251577337b1e1d Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Lars Volker Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior: left shift of negative values
Impala Public Jenkins has submitted this change and it was merged. Change subject: IMPALA-5031: Remove undefined behavior: left shift of negative values .. IMPALA-5031: Remove undefined behavior: left shift of negative values Left shift of negative values is undefined According to C++14 section 5.8 [expr.shift], paragraph 2. Change-Id: I9d965c3950a878c0a41b374d51bbe2e5705b3360 Reviewed-on: http://gerrit.cloudera.org:8080/6491 Reviewed-by: Dan Hecht Reviewed-by: Tim Armstrong Tested-by: Impala Public Jenkins --- M be/src/exec/read-write-util.cc M be/src/exec/zigzag-test.cc M be/src/exprs/bit-byte-functions-ir.cc 3 files changed, 6 insertions(+), 4 deletions(-) Approvals: Impala Public Jenkins: Verified Tim Armstrong: Looks good to me, approved Dan Hecht: Looks good to me, but someone else must approve -- To view, visit http://gerrit.cloudera.org:8080/6491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I9d965c3950a878c0a41b374d51bbe2e5705b3360 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-5132: Fix ASAN use after free in timezone db
Zach Amsden has uploaded a new change for review. http://gerrit.cloudera.org:8080/6503 Change subject: IMPALA-5132: Fix ASAN use after free in timezone_db .. IMPALA-5132: Fix ASAN use after free in timezone_db The issue is that the string temporary returned by .string goes out of scope immediately after being created. Also, the API to mkstemp is unclear on whether it modifies the string in place. Just strdup() this to be safe - this is not performance critical code. Change-Id: I490f741403ea2004bc51394aa1251577337b1e1d --- M be/src/exprs/timezone_db.cc 1 file changed, 4 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/03/6503/1 -- To view, visit http://gerrit.cloudera.org:8080/6503 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I490f741403ea2004bc51394aa1251577337b1e1d Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior: left shift of negative values
Impala Public Jenkins has posted comments on this change. Change subject: IMPALA-5031: Remove undefined behavior: left shift of negative values .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/6491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9d965c3950a878c0a41b374d51bbe2e5705b3360 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4758: (2/2) Impala-side changes to build with latest gutil
Henry Robinson has posted comments on this change. Change subject: IMPALA-4758: (2/2) Impala-side changes to build with latest gutil .. Patch Set 9: (2 comments) Thanks for the quick reviews! http://gerrit.cloudera.org:8080/#/c/5688/9/be/src/util/time.h File be/src/util/time.h: Line 36: return ts.tv_sec * NANOS_PER_SEC + ts.tv_nsec; > are you sure impala doesn't have a commit that introduced GetMonoTimeNanos( I think we did add it for MacOS (see https://github.com/apache/incubator-impala/commit/aeadd326a982af0f2ac8d55e425248a8bc7babfe), and that added GetMonoTimeNanos() for both platforms. I took the view that it's easier to maintain Impala-side dependencies for now, and that the MacOS port is effectively dead at this point. http://gerrit.cloudera.org:8080/#/c/5688/9/cmake_modules/kudu_cmake_fns.txt File cmake_modules/kudu_cmake_fns.txt: Line 64: endif() > what do these two changes do and why are they needed now? The first change adds custom compile flags to this target. This is needed to suppress some spurious warnings. The second change isn't strictly needed here, but since gutil is the first user of ADD_EXPORTABLE_LIBRARY I figured I'd include the fix at the earliest point. This allows CMake to properly handle transitive dependencies, so we don't need to 'flatten' all the deps of, e.g., kudu_util when linking against it. -- To view, visit http://gerrit.cloudera.org:8080/5688 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4ac21d7d6401f21fcdfdd1132b8f322bfba4bb80 Gerrit-PatchSet: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4758: (2/2) Impala-side changes to build with latest gutil
Impala Public Jenkins has posted comments on this change. Change subject: IMPALA-4758: (2/2) Impala-side changes to build with latest gutil .. Patch Set 9: Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/420/ -- To view, visit http://gerrit.cloudera.org:8080/5688 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4ac21d7d6401f21fcdfdd1132b8f322bfba4bb80 Gerrit-PatchSet: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4758: (2/2) Impala-side changes to build with latest gutil
Dan Hecht has posted comments on this change. Change subject: IMPALA-4758: (2/2) Impala-side changes to build with latest gutil .. Patch Set 9: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/5688/9/be/src/util/time.h File be/src/util/time.h: Line 36: return ts.tv_sec * NANOS_PER_SEC + ts.tv_nsec; are you sure impala doesn't have a commit that introduced GetMonoTimeNanos() that we should just cherry-pick? It might have been done that way to get it working on macos. If we don't, this is fine (though I don't see why kudu would have deleted that code). http://gerrit.cloudera.org:8080/#/c/5688/9/cmake_modules/kudu_cmake_fns.txt File cmake_modules/kudu_cmake_fns.txt: Line 64: endif() what do these two changes do and why are they needed now? -- To view, visit http://gerrit.cloudera.org:8080/5688 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4ac21d7d6401f21fcdfdd1132b8f322bfba4bb80 Gerrit-PatchSet: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4758: (1/2) Update gutil/ from Kudu@a1bfd7b
Dan Hecht has posted comments on this change. Change subject: IMPALA-4758: (1/2) Update gutil/ from Kudu@a1bfd7b .. Patch Set 8: Code-Review+2 (2 comments) > (4 comments) > > I don't think there's a new upstream version of this code in Kudu > (which comes from https://chromium.googlesource.com/chromium/src/base/). > But they made some local changes that were necessary to keep the > kudu library dependencies compiling. > Okay. I wasn't sure given some of the large deletions (like sparsetable.h). So you're saying that kudu deleted that code from their gutils, it wasn't deleted upstream). > I think having gutil as a toolchain dependency might make some > sense in the future, so that it's easier to replay the patches we > want if we ever upgrade the base code. > > For now, I've split the two commits a bit better: this commit only > imports the code (and touches CMakeLists.txt). The following commit > makes all the code changes required to have it compile. That way > only that commit needs to be checked in the future if we have to do > this again. Okay, let me take a quick look at the second patch again given some code has moved between them. http://gerrit.cloudera.org:8080/#/c/5687/7/be/src/gutil/gscoped_ptr.h File be/src/gutil/gscoped_ptr.h: PS7, Line 112: kudu > It happened in this kudu commit: It's just surprising to impala developers to see this, if they don't know this full history. you'd ask yourself, "what does this code have to do with kudu?" and the answer is "nothing". I'm fine with leaving it, just wasn't sure what was going on with it. http://gerrit.cloudera.org:8080/#/c/5687/7/be/src/gutil/port.h File be/src/gutil/port.h: PS7, Line 939: kudu > Came with the bulk-import. Since it's commented out, I didn't investigate t okay. similar to kudu namespace, this is just another thing that will be confusing to impala developers, but given how remote the code is, i'm not overly concerned. mainly just wanted to verify there wasn't a botched merge here or something. -- To view, visit http://gerrit.cloudera.org:8080/5687 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic708a9c4e76ede17af9b06e0a0a8e9ae7d357960 Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5130: fix race in MemTracker::EnableReservationReporting()
Tim Armstrong has uploaded a new change for review. http://gerrit.cloudera.org:8080/6502 Change subject: IMPALA-5130: fix race in MemTracker::EnableReservationReporting() .. IMPALA-5130: fix race in MemTracker::EnableReservationReporting() MemTracker::LogUsage() and MemTracker::EnableReservationReporting() could race, with LogUsage() seeing a partially-constructed 'reservation_counters_' value and crashing. This patch fixes that issue by atomically swapping in 'reservation_counters_' so that no threads see a partially-constructed value. While we're here, swap boost::mutex for the higher-performance SpinLock. Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2 --- M be/src/common/atomic.h M be/src/runtime/mem-tracker.cc M be/src/runtime/mem-tracker.h 3 files changed, 37 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/6502/1 -- To view, visit http://gerrit.cloudera.org:8080/6502 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I2434c952d97c46040e29fca2327c244dd30599d2 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong
[Impala-ASF-CR] IMPALA-4758: (1/2) Update gutil/ from Kudu@a1bfd7b
Henry Robinson has posted comments on this change. Change subject: IMPALA-4758: (1/2) Update gutil/ from Kudu@a1bfd7b .. Patch Set 7: (4 comments) I don't think there's a new upstream version of this code in Kudu (which comes from https://chromium.googlesource.com/chromium/src/base/). But they made some local changes that were necessary to keep the kudu library dependencies compiling. I think having gutil as a toolchain dependency might make some sense in the future, so that it's easier to replay the patches we want if we ever upgrade the base code. For now, I've split the two commits a bit better: this commit only imports the code (and touches CMakeLists.txt). The following commit makes all the code changes required to have it compile. That way only that commit needs to be checked in the future if we have to do this again. http://gerrit.cloudera.org:8080/#/c/5687/7/be/src/gutil/bits.cc File be/src/gutil/bits.cc: Line 88: } > why did these move from the header? I moved our implementation into our BitUtil class. These went back to the .cc file where they were originally. http://gerrit.cloudera.org:8080/#/c/5687/7/be/src/gutil/gscoped_ptr.h File be/src/gutil/gscoped_ptr.h: PS7, Line 112: kudu > is that intentional? It happened in this kudu commit: https://github.com/apache/kudu/commit/bb6da1946ea90e3b64bb7a38d9e5751cc95c557b "gutil: move classes used by exported client into kudu namespace" To me it's unfortunate but benign. Since the kutil libraries use these classes (much more than we do), we'd have to change the namespacing either here or there. I think, but am prepared to be convinced otherwise, that it's better to have as few changes to these bulk imports as possible. http://gerrit.cloudera.org:8080/#/c/5687/7/be/src/gutil/port.h File be/src/gutil/port.h: PS7, Line 939: kudu > is that expected? Came with the bulk-import. Since it's commented out, I didn't investigate too deeply (again, it's a place where fidelity to the source probably outweighs cleanliness concerns). http://gerrit.cloudera.org:8080/#/c/5687/7/be/src/gutil/walltime.h File be/src/gutil/walltime.h: PS7, Line 83: 1e6 > we had intentionally changed these so that we'd be using integer operations Done. -- To view, visit http://gerrit.cloudera.org:8080/5687 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic708a9c4e76ede17af9b06e0a0a8e9ae7d357960 Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4758: (2/2) Impala-side changes to build with latest gutil
Hello Impala Public Jenkins, Jim Apple, Dan Hecht, Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5688 to look at the new patch set (#9). Change subject: IMPALA-4758: (2/2) Impala-side changes to build with latest gutil .. IMPALA-4758: (2/2) Impala-side changes to build with latest gutil Meant to be taken as a whole with the previous commit. This patch makes the necessary code changes to Impala and the gutil/ library to fix all compilation errors. Future upgrades to gutil/ should redo the work in this commit. * Remove kudu/ include prefix with command: git grep -l "include \"kudu/" | xargs sed -i 's/include \"kudu\//include \"/g' * Change KUDU_GUTIL_* guards to be GUTIL_* git grep -l KUDU_GUTIL | xargs sed -i 's/KUDU_GUTIL/GUTIL/g' * Replace glog/logging.h with common/logging.h git grep -l "glog/logging" | xargs sed -i 's/glog\/logging/common\/logging/g' * Provide our own implementation of since-removed MonotonicNanos() * Reinstate COMPILE_FLAGS argument to ADD_EXPORTABLE_LIBRARY, used by gutil. * Replay overwritten parts of following commits: a7c3f30 - Remove AMD Opteron Rev E workaround from atomicops 54194af - IMPALA-4631: don't use floating point operations for time unit conversions 152c586 - Improve AtomicInt abstraction and implementation * Comment out non-compiling deprecated function definitions in numbers.h * Overwrite changes from 92fafa "Use more efficient gutil implementation of Log2Ceiling" in favour of implementing them in Impala code only. * Couple of misc fixes. Change-Id: I4ac21d7d6401f21fcdfdd1132b8f322bfba4bb80 --- M be/src/exec/hash-table.inline.h M be/src/gutil/atomic_refcount.h M be/src/gutil/atomicops-internals-x86.cc M be/src/gutil/atomicops-internals-x86.h M be/src/gutil/atomicops.h M be/src/gutil/auxiliary/atomicops-internals-arm-generic.h M be/src/gutil/auxiliary/atomicops-internals-arm-v6plus.h M be/src/gutil/auxiliary/atomicops-internals-windows.h M be/src/gutil/basictypes.h M be/src/gutil/bind.h M be/src/gutil/bind_helpers.h M be/src/gutil/bind_internal.h M be/src/gutil/bits.cc M be/src/gutil/bits.h M be/src/gutil/callback.h M be/src/gutil/callback_forward.h M be/src/gutil/callback_internal.cc M be/src/gutil/callback_internal.h M be/src/gutil/casts.h M be/src/gutil/charmap.h M be/src/gutil/cpu.cc M be/src/gutil/cycleclock-inl.h M be/src/gutil/endian.h M be/src/gutil/fixedarray.h M be/src/gutil/gscoped_ptr.h M be/src/gutil/hash/builtin_type_hash.h M be/src/gutil/hash/city.cc M be/src/gutil/hash/city.h M be/src/gutil/hash/hash.cc M be/src/gutil/hash/hash.h M be/src/gutil/hash/hash128to64.h M be/src/gutil/hash/jenkins.cc M be/src/gutil/hash/jenkins.h M be/src/gutil/hash/jenkins_lookup2.h M be/src/gutil/hash/legacy_hash.h M be/src/gutil/hash/string_hash.h M be/src/gutil/int128.cc M be/src/gutil/int128.h M be/src/gutil/macros.h M be/src/gutil/manual_constructor.h M be/src/gutil/map-util.h M be/src/gutil/mathlimits.cc M be/src/gutil/once.cc M be/src/gutil/once.h M be/src/gutil/paranoid.h M be/src/gutil/port.h M be/src/gutil/raw_scoped_refptr_mismatch_checker.h M be/src/gutil/ref_counted.cc M be/src/gutil/ref_counted.h M be/src/gutil/ref_counted_memory.cc M be/src/gutil/ref_counted_memory.h M be/src/gutil/singleton.h M be/src/gutil/spinlock.cc M be/src/gutil/spinlock.h M be/src/gutil/spinlock_internal.cc M be/src/gutil/spinlock_internal.h M be/src/gutil/spinlock_linux-inl.h M be/src/gutil/stl_util.h M be/src/gutil/stringprintf.cc M be/src/gutil/stringprintf.h M be/src/gutil/strings/ascii_ctype.cc M be/src/gutil/strings/charset.cc M be/src/gutil/strings/charset.h M be/src/gutil/strings/escaping.cc M be/src/gutil/strings/escaping.h M be/src/gutil/strings/fastmem.h M be/src/gutil/strings/human_readable.cc M be/src/gutil/strings/human_readable.h M be/src/gutil/strings/join.cc M be/src/gutil/strings/join.h M be/src/gutil/strings/memutil.cc M be/src/gutil/strings/memutil.h M be/src/gutil/strings/numbers.cc M be/src/gutil/strings/numbers.h M be/src/gutil/strings/serialize.cc M be/src/gutil/strings/serialize.h M be/src/gutil/strings/split.cc M be/src/gutil/strings/split.h M be/src/gutil/strings/split_internal.h M be/src/gutil/strings/strcat.cc M be/src/gutil/strings/strcat.h M be/src/gutil/strings/string_util-test.cc M be/src/gutil/strings/stringpiece.cc M be/src/gutil/strings/stringpiece.h M be/src/gutil/strings/strip.cc M be/src/gutil/strings/strip.h M be/src/gutil/strings/substitute.cc M be/src/gutil/strings/substitute.h M be/src/gutil/strings/util.cc M be/src/gutil/strings/util.h M be/src/gutil/strtoint.cc M be/src/gutil/strtoint.h M be/src/gutil/synchronization_profiling.h M be/src/gutil/sysinfo.cc M be/src/gutil/threading/thread_collision_warner.cc M be/src/gutil/threading/thread_collision_warner.h M be/src/gutil/type_traits.h M be/src/gutil/walltime.cc M be/src/gutil/walltime.h M be/src/runtime/buffered-tuple-stream-test.cc M be/src/runtime/buf
[Impala-ASF-CR] IMPALA-4758: (1/2) Update gutil/ from Kudu@a1bfd7b
Hello Tim Armstrong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/5687 to look at the new patch set (#8). Change subject: IMPALA-4758: (1/2) Update gutil/ from Kudu@a1bfd7b .. IMPALA-4758: (1/2) Update gutil/ from Kudu@a1bfd7b * Copy gutil from Kudu * Minimal changes to gutil/CMakeLists.txt Change-Id: Ic708a9c4e76ede17af9b06e0a0a8e9ae7d357960 --- M be/src/gutil/CMakeLists.txt D be/src/gutil/algorithm.h M be/src/gutil/atomic_refcount.h M be/src/gutil/atomicops-internals-powerpc.h A be/src/gutil/atomicops-internals-tsan.h M be/src/gutil/atomicops-internals-x86.cc M be/src/gutil/atomicops-internals-x86.h M be/src/gutil/atomicops.h M be/src/gutil/auxiliary/atomicops-internals-arm-generic.h M be/src/gutil/auxiliary/atomicops-internals-arm-v6plus.h D be/src/gutil/auxiliary/atomicops-internals-macosx.h M be/src/gutil/auxiliary/atomicops-internals-windows.h M be/src/gutil/basictypes.h A be/src/gutil/bind.h A be/src/gutil/bind.h.pump A be/src/gutil/bind_helpers.h A be/src/gutil/bind_internal.h A be/src/gutil/bind_internal.h.pump M be/src/gutil/bits.cc M be/src/gutil/bits.h A be/src/gutil/callback.h A be/src/gutil/callback.h.pump A be/src/gutil/callback_forward.h A be/src/gutil/callback_internal.cc A be/src/gutil/callback_internal.h M be/src/gutil/casts.h M be/src/gutil/charmap.h A be/src/gutil/cpu.cc A be/src/gutil/cpu.h M be/src/gutil/cycleclock-inl.h R be/src/gutil/dynamic_annotations.c M be/src/gutil/dynamic_annotations.h M be/src/gutil/endian.h M be/src/gutil/fixedarray.h M be/src/gutil/gscoped_ptr.h M be/src/gutil/hash/builtin_type_hash.h M be/src/gutil/hash/city.cc M be/src/gutil/hash/city.h M be/src/gutil/hash/hash.cc M be/src/gutil/hash/hash.h M be/src/gutil/hash/hash128to64.h M be/src/gutil/hash/jenkins.cc M be/src/gutil/hash/jenkins.h M be/src/gutil/hash/jenkins_lookup2.h M be/src/gutil/hash/legacy_hash.h M be/src/gutil/hash/string_hash.h M be/src/gutil/int128.cc M be/src/gutil/int128.h M be/src/gutil/integral_types.h M be/src/gutil/linux_syscall_support.h M be/src/gutil/logging-inl.h M be/src/gutil/macros.h M be/src/gutil/manual_constructor.h M be/src/gutil/map-util.h M be/src/gutil/mathlimits.cc M be/src/gutil/mathlimits.h M be/src/gutil/move.h M be/src/gutil/once.cc M be/src/gutil/once.h M be/src/gutil/paranoid.h M be/src/gutil/port.h D be/src/gutil/proto/types.pb.cc D be/src/gutil/proto/types.pb.h D be/src/gutil/proto/types.proto A be/src/gutil/raw_scoped_refptr_mismatch_checker.h A be/src/gutil/ref_counted.cc A be/src/gutil/ref_counted.h A be/src/gutil/ref_counted_memory.cc A be/src/gutil/ref_counted_memory.h M be/src/gutil/singleton.h D be/src/gutil/sparsetable.h M be/src/gutil/spinlock.cc M be/src/gutil/spinlock.h M be/src/gutil/spinlock_internal.cc M be/src/gutil/spinlock_internal.h M be/src/gutil/spinlock_linux-inl.h M be/src/gutil/spinlock_win32-inl.h M be/src/gutil/stl_util.h M be/src/gutil/stringprintf.cc M be/src/gutil/stringprintf.h M be/src/gutil/strings/ascii_ctype.cc M be/src/gutil/strings/charset.cc M be/src/gutil/strings/charset.h M be/src/gutil/strings/escaping.cc M be/src/gutil/strings/escaping.h M be/src/gutil/strings/fastmem.h M be/src/gutil/strings/human_readable.cc M be/src/gutil/strings/human_readable.h M be/src/gutil/strings/join.cc M be/src/gutil/strings/join.h M be/src/gutil/strings/memutil.cc M be/src/gutil/strings/memutil.h M be/src/gutil/strings/numbers.cc M be/src/gutil/strings/numbers.h M be/src/gutil/strings/serialize.cc M be/src/gutil/strings/serialize.h M be/src/gutil/strings/split.cc M be/src/gutil/strings/split.h M be/src/gutil/strings/split_internal.h M be/src/gutil/strings/strcat.cc M be/src/gutil/strings/strcat.h A be/src/gutil/strings/string_util-test.cc M be/src/gutil/strings/stringpiece.cc M be/src/gutil/strings/stringpiece.h M be/src/gutil/strings/strip.cc M be/src/gutil/strings/strip.h M be/src/gutil/strings/substitute.cc M be/src/gutil/strings/substitute.h M be/src/gutil/strings/util.cc M be/src/gutil/strings/util.h M be/src/gutil/strtoint.cc M be/src/gutil/strtoint.h M be/src/gutil/synchronization_profiling.h M be/src/gutil/sysinfo.cc M be/src/gutil/sysinfo.h M be/src/gutil/template_util.h A be/src/gutil/threading/thread_collision_warner.cc A be/src/gutil/threading/thread_collision_warner.h M be/src/gutil/type_traits.h R be/src/gutil/utf/rune.c M be/src/gutil/walltime.cc M be/src/gutil/walltime.h 122 files changed, 9,196 insertions(+), 4,102 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/5687/8 -- To view, visit http://gerrit.cloudera.org:8080/5687 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic708a9c4e76ede17af9b06e0a0a8e9ae7d357960 Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] [DOCS] Remove references to DSSD storage appliance
Michael Brown has posted comments on this change. Change subject: [DOCS] Remove references to DSSD storage appliance .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/6412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9e47fae9b098c6be85a769fdbac8ea2b87d6c167 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: John Russell Gerrit-Reviewer: Greg Rahn Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Michael Brown Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4701: make distcc work reliably with clang
Lars Volker has posted comments on this change. Change subject: IMPALA-4701: make distcc work reliably with clang .. Patch Set 7: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/6493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I199b245fb14b6c3484b66339a7d4b37d74755af7 Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4701: make distcc work reliably with clang
Hello Lars Volker, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6493 to look at the new patch set (#7). Change subject: IMPALA-4701: make distcc work reliably with clang .. IMPALA-4701: make distcc work reliably with clang The previous compiler-switching method in distcc had some problems: * It duplicated logic from CMakeLists.txt in choosing flags to pass to clang * In order to switch to ASAN, you needed to remember to change distcc settings. * The wrapper script approach interacted badly with ccache - GCC and Clang-generated artifacts would be treated as equivalent by ccache, meaning that if you accidentally build with the wrong compiler setting and the artifacts got into the cache you needed to clear ccache. Instead of using environment variables to set the compiler, we now pass the compiler as an argument to distcc.sh and set things up in CMakeLists.txt the same way as ccache. Switching to/from clang builds now requires no extra step (aside from cleaning out the cmake-generated files with clean.sh). Also changes the name of the config file Testing: Tested switching between debug and asan builds locally. Change-Id: I199b245fb14b6c3484b66339a7d4b37d74755af7 --- M be/CMakeLists.txt M bin/clean.sh M bin/distcc/README.md M bin/distcc/distcc.sh M bin/distcc/distcc_env.sh 5 files changed, 47 insertions(+), 54 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/6493/7 -- To view, visit http://gerrit.cloudera.org:8080/6493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I199b245fb14b6c3484b66339a7d4b37d74755af7 Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4701: make distcc work reliably with clang
Tim Armstrong has posted comments on this change. Change subject: IMPALA-4701: make distcc work reliably with clang .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/6493/5/bin/distcc/distcc_env.sh File bin/distcc/distcc_env.sh: Line 120: -not -path '*thirdparty*' | xargs rm -rf > -execdir is the new -exec: I switched to -exec. I couldn't get -execdir to work reliably, I think because we're trying to delete directories. I don't think there are any security implications since we're only working within the Impala tree. -- To view, visit http://gerrit.cloudera.org:8080/6493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I199b245fb14b6c3484b66339a7d4b37d74755af7 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause
Lars Volker has posted comments on this change. Change subject: IMPALA-4166: Add SORT BY sql clause .. Patch Set 2: PS2 adds a file I had forgotten to "git add" in PS1. -- To view, visit http://gerrit.cloudera.org:8080/6495 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I08834f38a941786ab45a4381c2732d929a934f75 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker Gerrit-Reviewer: Lars Volker Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4166: Add SORT BY sql clause
Lars Volker has uploaded a new patch set (#2). Change subject: IMPALA-4166: Add SORT BY sql clause .. IMPALA-4166: Add SORT BY sql clause This change adds support for adding SORT BY (...) clauses to CREATE TABLE and ALTER TABLE statements. The columns in the SORT BY clause are stored in the 'sort.by.columns' table property and will result in an additional SORT node being added to the plan before the final table sink. Specifying SORT BY columns also enables clustering during inserts, so the SORT node will contain all partitioning columns first, followed by the SORT BY columns. This change introduces a new TablePropertyAnalyzer class to collect various methods used to analyze table properties. Change-Id: I08834f38a941786ab45a4381c2732d929a934f75 --- M common/thrift/JniCatalog.thrift M fe/src/main/cup/sql-parser.cup M fe/src/main/java/org/apache/impala/analysis/AlterTableSetTblProperties.java M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/BaseTableRef.java M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeStmt.java M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java M fe/src/main/java/org/apache/impala/analysis/TableDef.java A fe/src/main/java/org/apache/impala/analysis/TablePropertyAnalyzer.java M fe/src/main/java/org/apache/impala/catalog/Column.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/planner/Planner.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/jflex/sql-scanner.flex M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java M fe/src/test/java/org/apache/impala/planner/PlannerTest.java M testdata/workloads/functional-planner/queries/PlannerTest/ddl.test A testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test M testdata/workloads/functional-planner/queries/PlannerTest/insert.test M testdata/workloads/functional-query/queries/QueryTest/alter-table.test M testdata/workloads/functional-query/queries/QueryTest/create-table-as-select.test M testdata/workloads/functional-query/queries/QueryTest/create-table-like-file.test M testdata/workloads/functional-query/queries/QueryTest/create-table-like-table.test M testdata/workloads/functional-query/queries/QueryTest/create-table.test M testdata/workloads/functional-query/queries/QueryTest/show-create-table.test 29 files changed, 730 insertions(+), 51 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/95/6495/2 -- To view, visit http://gerrit.cloudera.org:8080/6495 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I08834f38a941786ab45a4381c2732d929a934f75 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Lars Volker Gerrit-Reviewer: Lars Volker
[Impala-ASF-CR] IMPALA-4701: make distcc work reliably with clang
Jim Apple has posted comments on this change.
Change subject: IMPALA-4701: make distcc work reliably with clang
..
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6493/5/bin/distcc/distcc_env.sh
File bin/distcc/distcc_env.sh:
Line 120: -not -path '*thirdparty*' | xargs rm -rf
> I think it would be safer to use find's -print0 parameter and xargs -0 here
-execdir is the new -exec:
-execdir command ;
-execdir command {} +
Like -exec, but the specified command is run from the
subdirectory containing the matched file, which is not
normally the directory in which you started find. This a much
more secure method for invoking commands, as
it avoids race conditions during resolution of the paths to
the matched files.
--
To view, visit http://gerrit.cloudera.org:8080/6493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I199b245fb14b6c3484b66339a7d4b37d74755af7
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Tim Armstrong
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4701: make distcc work reliably with clang
Lars Volker has posted comments on this change.
Change subject: IMPALA-4701: make distcc work reliably with clang
..
Patch Set 5:
(1 comment)
Apologies for leaving yet another comment. Feel free to ignore it.
http://gerrit.cloudera.org:8080/#/c/6493/5/bin/distcc/distcc_env.sh
File bin/distcc/distcc_env.sh:
Line 120: -not -path '*thirdparty*' | xargs rm -rf
> While I'm here, this wasn't updated to take into account the toolchain
I think it would be safer to use find's -print0 parameter and xargs -0 here,
otherwise files with spaces will be an issue.
On second thought, you could do something like
find . -name "FILE-TO-FIND" -exec rm -rf {} \;
and do away with xargs altogether.
--
To view, visit http://gerrit.cloudera.org:8080/6493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I199b245fb14b6c3484b66339a7d4b37d74755af7
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Jim Apple
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Tim Armstrong
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4041: Limit catalog and admission control updates to coordinators
Impala Public Jenkins has posted comments on this change. Change subject: IMPALA-4041: Limit catalog and admission control updates to coordinators .. Patch Set 6: Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/419/ -- To view, visit http://gerrit.cloudera.org:8080/6344 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5f2c74abdbcd60ac050efa323616bd41182ceff3 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4041: Limit catalog and admission control updates to coordinators
Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-4041: Limit catalog and admission control updates to coordinators .. Patch Set 6: Code-Review+2 Keep Henry's +2 -- To view, visit http://gerrit.cloudera.org:8080/6344 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5f2c74abdbcd60ac050efa323616bd41182ceff3 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4041: Limit catalog and admission control updates to coordinators
Hello Marcel Kornacker, Henry Robinson, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6344 to look at the new patch set (#6). Change subject: IMPALA-4041: Limit catalog and admission control updates to coordinators .. IMPALA-4041: Limit catalog and admission control updates to coordinators With this commit we add the ability to limit catalog updates to a limited set of coordinator nodes. A new startup option, termed 'is_coordinator' is added to indicate if a node is a coordinator. Coordinators accept connections through HS2 and Beeswax interfaces and can also participate in query execution. Non-coordinator nodes do not receive catalog updates from the statestore, do not initialize a query scheduler and cannot accept Beeswax and HS2 client connections. Testing: - Added a custom cluster test that launches a cluster in which the number of coordinators is less than the cluster size and runs a number of smoke queries. - Successfully run exhaustive tests. Change-Id: I5f2c74abdbcd60ac050efa323616bd41182ceff3 --- M be/src/runtime/exec-env.cc M be/src/runtime/exec-env.h M be/src/scheduling/admission-controller.cc M be/src/scheduling/admission-controller.h M be/src/scheduling/scheduler-test-util.cc M be/src/scheduling/scheduler-test.cc M be/src/scheduling/scheduler.cc M be/src/scheduling/scheduler.h M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/service/impalad-main.cc M be/src/service/query-exec-state.cc M be/src/testutil/in-process-servers.h M be/src/util/webserver.cc M bin/start-impala-cluster.py M tests/common/custom_cluster_test_suite.py M tests/common/impala_service.py A tests/custom_cluster/test_coordinators.py M www/root.tmpl 19 files changed, 435 insertions(+), 235 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/44/6344/6 -- To view, visit http://gerrit.cloudera.org:8080/6344 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5f2c74abdbcd60ac050efa323616bd41182ceff3 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs
[Impala-ASF-CR] IMPALA-4041: Limit catalog and admission control updates to coordinators
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-4041: Limit catalog and admission control updates to
coordinators
..
Patch Set 5:
(6 comments)
Thanks Henry!
http://gerrit.cloudera.org:8080/#/c/6344/5/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
PS5, Line 180: node can coordinate queries.
> maybe tighten this up:
Done
http://gerrit.cloudera.org:8080/#/c/6344/5/be/src/service/impala-server.h
File be/src/service/impala-server.h:
Line 949:
> comment?
Done
http://gerrit.cloudera.org:8080/#/c/6344/5/be/src/service/query-exec-state.cc
File be/src/service/query-exec-state.cc:
PS5, Line 461: if (!FLAGS_disable_admission_control) {
> again, it seems better to check to see if admission_controller() != nullptr
Done
http://gerrit.cloudera.org:8080/#/c/6344/5/be/src/util/webserver.cc
File be/src/util/webserver.cc:
PS5, Line 192: FLAGS_is_coordinator
> it would be neater to do something like:
Done
PS5, Line 192: Worker
> nit: dunno how much we want to bikeshed this, but I prefer "Executor" to "W
Renamed "worker" to "executor". Also, "coordinator" now is "coordinator +
executor".
http://gerrit.cloudera.org:8080/#/c/6344/5/www/root.tmpl
File www/root.tmpl:
Line 21: Impala Server Mode
> This template gets used for catalog and statestored as well. You need to ch
Good point, forgot about it. Fixed it.
--
To view, visit http://gerrit.cloudera.org:8080/6344
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5f2c74abdbcd60ac050efa323616bd41182ceff3
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Dan Hecht
Gerrit-Reviewer: Dimitris Tsirogiannis
Gerrit-Reviewer: Henry Robinson
Gerrit-Reviewer: Marcel Kornacker
Gerrit-Reviewer: Matthew Jacobs
Gerrit-HasComments: Yes
[Impala-ASF-CR](asf-site) Update Impala Jira link to ASF's Jira.
Michael Brown has submitted this change and it was merged. Change subject: Update Impala Jira link to ASF's Jira. .. Update Impala Jira link to ASF's Jira. Change-Id: Ia903898c4e31fd0f4820048244e23ffa638dab4a Reviewed-on: http://gerrit.cloudera.org:8080/6499 Reviewed-by: Henry Robinson Tested-by: Michael Brown --- M community.html 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Michael Brown: Verified Henry Robinson: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/6499 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ia903898c4e31fd0f4820048244e23ffa638dab4a Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: asf-site Gerrit-Owner: Michael Brown Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Michael Brown
[Impala-ASF-CR](asf-site) Update Impala Jira link to ASF's Jira.
Michael Brown has posted comments on this change. Change subject: Update Impala Jira link to ASF's Jira. .. Patch Set 1: Verified+1 I verified by looking at community.html in the links2 browser, navigating to the issues link, and following the link. -- To view, visit http://gerrit.cloudera.org:8080/6499 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia903898c4e31fd0f4820048244e23ffa638dab4a Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: asf-site Gerrit-Owner: Michael Brown Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Michael Brown Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior: left shift of negative values
Impala Public Jenkins has posted comments on this change. Change subject: IMPALA-5031: Remove undefined behavior: left shift of negative values .. Patch Set 1: Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/418/ -- To view, visit http://gerrit.cloudera.org:8080/6491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9d965c3950a878c0a41b374d51bbe2e5705b3360 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5031: Remove undefined behavior: left shift of negative values
Tim Armstrong has posted comments on this change. Change subject: IMPALA-5031: Remove undefined behavior: left shift of negative values .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/6491/1/be/src/exec/read-write-util.cc File be/src/exec/read-write-util.cc: Line 96: uint32_t uinteger = (static_cast(integer) << 1) ^ (integer >> 31); > That's what I thought, too, but it doesn't work: the right shift depends on Ah got it -- To view, visit http://gerrit.cloudera.org:8080/6491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9d965c3950a878c0a41b374d51bbe2e5705b3360 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4318: Kudu support for CREATE EXTERNAL TABLE AS SELECT
Dan Hecht has posted comments on this change. Change subject: IMPALA-4318: Kudu support for CREATE EXTERNAL TABLE AS SELECT .. Patch Set 10: Code-Review+1 Thanks, the new commit message is much clearer. -- To view, visit http://gerrit.cloudera.org:8080/6261 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9aa82809a6c0c5e6386827314b7e5b520c1a6633 Gerrit-PatchSet: 10 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Zach Amsden Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5128: BE Test Infrastructure
Zach Amsden has uploaded a new patch set (#2). Change subject: IMPALA-5128: BE Test Infrastructure .. IMPALA-5128: BE Test Infrastructure Investigating a crash in backend exprs test and this test takes a long time to run. Lets add the notion of exhaustive tests to the be tests as well and skip tests which take a longer amount of time. This diff adds a new boolean flag to all tests, -run-exhaustive, which can be used to enable extra testing, and makes the default test run use this only if exhaustive mode has been requested. Change-Id: I71fe6de39dff21b21d322daf0232b0578bdff297 --- M be/CMakeLists.txt M be/src/exprs/expr-test.cc M be/src/testutil/CMakeLists.txt A be/src/testutil/gtest-util.cc M be/src/testutil/gtest-util.h M bin/run-all-tests.sh M bin/run-backend-tests.sh 7 files changed, 432 insertions(+), 379 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/77/6477/2 -- To view, visit http://gerrit.cloudera.org:8080/6477 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I71fe6de39dff21b21d322daf0232b0578bdff297 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Michael Ho
[Impala-ASF-CR](asf-site) Update Impala Jira link to ASF's Jira.
Henry Robinson has posted comments on this change. Change subject: Update Impala Jira link to ASF's Jira. .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6499 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia903898c4e31fd0f4820048244e23ffa638dab4a Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: asf-site Gerrit-Owner: Michael Brown Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Jim Apple Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4318: Kudu support for CREATE EXTERNAL TABLE AS SELECT
Hello Matthew Jacobs, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6261 to look at the new patch set (#10). Change subject: IMPALA-4318: Kudu support for CREATE EXTERNAL TABLE AS SELECT .. IMPALA-4318: Kudu support for CREATE EXTERNAL TABLE AS SELECT Create external table can be used for two different operations, creating a new table by selecting data from another, and importing an existing unmanaged external table. Previously, for Kudu tables, it was only possible to issue CTAS for managed tables, and external tables were assumed to be imports of already existing data. This change allows the creation of new, unmanaged (external) kudu tables from SELECT statements. In the catalog executor, we detect if a table creation is new by inferring that from the presence of column definitions, not by whether it is external / managed. Front-end code is changed to allow this through the parser. This means any attempts to specify table metadata for Kudu tables are now interpreted as a table creation. Testing: Manual testing, expanded ParserTest and AnalyzeDDLTest and added test cases to query_test to validate Kudu support. Ran the folowing query: create external table test2_name primary key(id) partition by hash(id) partitions 10 stored as kudu as select * from test_name; Which successfully created and inserted 3 rows of data. Tried many other things like changing primary keys and forms of partitioning. Change-Id: I9aa82809a6c0c5e6386827314b7e5b520c1a6633 --- M fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java M fe/src/test/java/org/apache/impala/analysis/ParserTest.java M shell/impala_shell.py M tests/query_test/test_kudu.py 8 files changed, 92 insertions(+), 33 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/61/6261/10 -- To view, visit http://gerrit.cloudera.org:8080/6261 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9aa82809a6c0c5e6386827314b7e5b520c1a6633 Gerrit-PatchSet: 10 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Zach Amsden
[Impala-ASF-CR] IMPALA-4318: Kudu support for CREATE EXTERNAL TABLE AS SELECT
Zach Amsden has posted comments on this change. Change subject: IMPALA-4318: Kudu support for CREATE EXTERNAL TABLE AS SELECT .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/6261/9//COMMIT_MSG Commit Message: Line 10: creating a new managed table, and importing an existing unmanaged > "Managed" is a synonym for "internal", and the opposite of "external". So, Code is right, comment is very confused. Let's try this: Create external table can be used for two different operations, creating a new table by selecting data from another, and importing an existing unmanaged external table. Previously, for Kudu tables, it was only possible to issue CTAS for managed tables, and external tables were assumed to be imports of already existing data. This change allows the creation of new, unmanaged (external) kudu tables from SELECT statements. In the catalog executor, we detect if a table creation is new by inferring that from the presence of column definitions, not by whether it is external / managed. Front-end code is changed to allow this through the parser. This means any attempts to specify table metadata for Kudu tables are now interpreted as a table creation. -- To view, visit http://gerrit.cloudera.org:8080/6261 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9aa82809a6c0c5e6386827314b7e5b520c1a6633 Gerrit-PatchSet: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Zach Amsden Gerrit-HasComments: Yes
[Impala-ASF-CR](asf-site) Update Impala Jira link to ASF's Jira.
Michael Brown has uploaded a new change for review. http://gerrit.cloudera.org:8080/6499 Change subject: Update Impala Jira link to ASF's Jira. .. Update Impala Jira link to ASF's Jira. Change-Id: Ia903898c4e31fd0f4820048244e23ffa638dab4a --- M community.html 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/6499/1 -- To view, visit http://gerrit.cloudera.org:8080/6499 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ia903898c4e31fd0f4820048244e23ffa638dab4a Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: asf-site Gerrit-Owner: Michael Brown
[Impala-ASF-CR] IMPALA-5127: Add history max option
Hello Sailesh Mukil, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6335 to look at the new patch set (#2). Change subject: IMPALA-5127: Add history_max option .. IMPALA-5127: Add history_max option Allow users to keep a longer history of queries if desired. I personally find it useful to keep a long history of queries to reference and want to bump this up to a very large value, but keep the default reasonable. Testing: Created .impalarc as follows, now getting more history saved. [impala] history_max=1000 Change-Id: Iaf65bbecb8fd7f1105aac62b6745d6125a603d7f --- M shell/impala_shell.py M shell/impala_shell_config_defaults.py 2 files changed, 21 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/6335/2 -- To view, visit http://gerrit.cloudera.org:8080/6335 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iaf65bbecb8fd7f1105aac62b6745d6125a603d7f Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Sailesh Mukil
[Impala-ASF-CR] IMPALA-4318: Kudu support for CREATE EXTERNAL TABLE AS SELECT
Dan Hecht has posted comments on this change. Change subject: IMPALA-4318: Kudu support for CREATE EXTERNAL TABLE AS SELECT .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/6261/9//COMMIT_MSG Commit Message: Line 10: creating a new managed table, and importing an existing unmanaged "Managed" is a synonym for "internal", and the opposite of "external". So, I don't understand what you mean by "creating a new managed table" when it comes to "create external table". The semantics for external tables is that DROP TABLE won't delete the backing data. That should be true for all external tables, whether Impala had to create the table or the table was pre-existing. So, what do you mean exactly by "managed" here? -- To view, visit http://gerrit.cloudera.org:8080/6261 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9aa82809a6c0c5e6386827314b7e5b520c1a6633 Gerrit-PatchSet: 9 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Zach Amsden Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Dimitris Tsirogiannis Gerrit-Reviewer: Marcel Kornacker Gerrit-Reviewer: Matthew Jacobs Gerrit-Reviewer: Thomas Tauber-Marshall Gerrit-Reviewer: Zach Amsden Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4758: (2/2) Impala-side changes to build with latest gutil
Dan Hecht has posted comments on this change. Change subject: IMPALA-4758: (2/2) Impala-side changes to build with latest gutil .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/5688 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4ac21d7d6401f21fcdfdd1132b8f322bfba4bb80 Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4758: (1/2) Update gutil/ from Kudu@a1bfd7b
Dan Hecht has posted comments on this change. Change subject: IMPALA-4758: (1/2) Update gutil/ from Kudu@a1bfd7b .. Patch Set 7: (4 comments) Do you know if kudu actually pulled in a newer version of this code from upstream, and if so, which version? http://gerrit.cloudera.org:8080/#/c/5687/7/be/src/gutil/bits.cc File be/src/gutil/bits.cc: Line 88: } why did these move from the header? http://gerrit.cloudera.org:8080/#/c/5687/7/be/src/gutil/gscoped_ptr.h File be/src/gutil/gscoped_ptr.h: PS7, Line 112: kudu is that intentional? http://gerrit.cloudera.org:8080/#/c/5687/7/be/src/gutil/port.h File be/src/gutil/port.h: PS7, Line 939: kudu is that expected? http://gerrit.cloudera.org:8080/#/c/5687/7/be/src/gutil/walltime.h File be/src/gutil/walltime.h: PS7, Line 83: 1e6 we had intentionally changed these so that we'd be using integer operations rather than floating point. can we cherry pick that commit back over? -- To view, visit http://gerrit.cloudera.org:8080/5687 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic708a9c4e76ede17af9b06e0a0a8e9ae7d357960 Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4701: make distcc work reliably with clang
Hello Lars Volker, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6493 to look at the new patch set (#6). Change subject: IMPALA-4701: make distcc work reliably with clang .. IMPALA-4701: make distcc work reliably with clang The previous compiler-switching method in distcc had some problems: * It duplicated logic from CMakeLists.txt in choosing flags to pass to clang * In order to switch to ASAN, you needed to remember to change distcc settings. * The wrapper script approach interacted badly with ccache - GCC and Clang-generated artifacts would be treated as equivalent by ccache, meaning that if you accidentally build with the wrong compiler setting and the artifacts got into the cache you needed to clear ccache. Instead of using environment variables to set the compiler, we now pass the compiler as an argument to distcc.sh and set things up in CMakeLists.txt the same way as ccache. Switching to/from clang builds now requires no extra step (aside from cleaning out the cmake-generated files with clean.sh). Also changes the name of the config file Testing: Tested switching between debug and asan builds locally. Change-Id: I199b245fb14b6c3484b66339a7d4b37d74755af7 --- M be/CMakeLists.txt M bin/distcc/README.md M bin/distcc/distcc.sh M bin/distcc/distcc_env.sh 4 files changed, 46 insertions(+), 53 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/6493/6 -- To view, visit http://gerrit.cloudera.org:8080/6493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I199b245fb14b6c3484b66339a7d4b37d74755af7 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-4701: make distcc work reliably with clang
Tim Armstrong has posted comments on this change. Change subject: IMPALA-4701: make distcc work reliably with clang .. Patch Set 6: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/6493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I199b245fb14b6c3484b66339a7d4b37d74755af7 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4701: make distcc work reliably with clang
Tim Armstrong has posted comments on this change. Change subject: IMPALA-4701: make distcc work reliably with clang .. Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/6493/5/bin/distcc/distcc.sh File bin/distcc/distcc.sh: Line 19: > Would it make sense to add "set -euo pipefail" here? Done http://gerrit.cloudera.org:8080/#/c/6493/5/bin/distcc/distcc_env.sh File bin/distcc/distcc_env.sh: Line 83: IMPALA_COMPILER_CONFIG_FILE="$IMPALA_HOME/.impala_compiler_opts" I ran into some issues locally because the old opts file wasn't compatible with the changed script. So I just removed Line 120: -not -path '*thirdparty*' | xargs rm -rf While I'm here, this wasn't updated to take into account the toolchain -- To view, visit http://gerrit.cloudera.org:8080/6493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I199b245fb14b6c3484b66339a7d4b37d74755af7 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4114: Port BufferedBlockMgr tests to buffer pool
Tim Armstrong has uploaded a new patch set (#2). Change subject: IMPALA-4114: Port BufferedBlockMgr tests to buffer pool .. IMPALA-4114: Port BufferedBlockMgr tests to buffer pool BufferedBlockMgr had a number of interesting backend tests that are still relevant to BufferPool. This commit copies them across and adapts them to BufferPool. This should bring the backend test coverage for BufferPool up to par with BufferedBlockMgr. Many tests weren't ported because they are not relevant or would duplicate other tests: * GetNewBlock* -> covered by PageCreation/BufferAllocation * Pin -> covered by Pin * Deletion/DeleteSingleBlocks -> all BufferPool tests cover deletion * Close -> BufferPool doesn't have "cancellation" * TransferBufferDuringWrite -> the API being tested is not present. Some of the deletion tests are the closest analogue. * WriteCompleteWithCancelledRuntimeState -> not relevant, BufferPool doesn't reference RuntimeState. * MultipleClients* -> we have many tests for the (very different) reservation mechanism * ClientOversubscription -> oversubscription is not supported * CreateDestroyMulti -> we don't support creation/destruction of buffer pools like this * AllocationErrorHandling -> redundant with WriteErrorBlacklist Change-Id: Ifb0221e8bea6f3b23b62d5094634d97562295ea3 --- M be/src/runtime/bufferpool/buffer-allocator.h M be/src/runtime/bufferpool/buffer-pool-internal.h M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/bufferpool/buffer-pool.cc M be/src/runtime/bufferpool/buffer-pool.h 5 files changed, 819 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/6498/2 -- To view, visit http://gerrit.cloudera.org:8080/6498 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ifb0221e8bea6f3b23b62d5094634d97562295ea3 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong
[Impala-ASF-CR] Port BufferedBlockMgr tests to buffer pool
Tim Armstrong has uploaded a new change for review. http://gerrit.cloudera.org:8080/6498 Change subject: Port BufferedBlockMgr tests to buffer pool .. Port BufferedBlockMgr tests to buffer pool BufferedBlockMgr had a number of interesting backend tests that are still relevant to BufferPool. This commit copies them across and adapts them to BufferPool. This should bring the backend test coverage for BufferPool up to par with BufferedBlockMgr. Many tests weren't ported because they are not relevant or would duplicate other tests: * GetNewBlock* -> covered by PageCreation/BufferAllocation * Pin -> covered by Pin * Deletion/DeleteSingleBlocks -> all BufferPool tests cover deletion * Close -> BufferPool doesn't have "cancellation" * TransferBufferDuringWrite -> the API being tested is not present. Some of the deletion tests are the closest analogue. * WriteCompleteWithCancelledRuntimeState -> not relevant, BufferPool doesn't reference RuntimeState. * MultipleClients* -> we have many tests for the (very different) reservation mechanism * ClientOversubscription -> oversubscription is not supported * CreateDestroyMulti -> we don't support creation/destruction of buffer pools like this * AllocationErrorHandling -> redundant with WriteErrorBlacklist Change-Id: Ifb0221e8bea6f3b23b62d5094634d97562295ea3 --- M be/src/runtime/bufferpool/buffer-allocator.h M be/src/runtime/bufferpool/buffer-pool-internal.h M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/bufferpool/buffer-pool.cc M be/src/runtime/bufferpool/buffer-pool.h 5 files changed, 819 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/6498/1 -- To view, visit http://gerrit.cloudera.org:8080/6498 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ifb0221e8bea6f3b23b62d5094634d97562295ea3 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong
[Impala-ASF-CR] IMPALA-3203: Part 2: per-core free lists in buffer pool
Tim Armstrong has uploaded a new patch set (#14). Change subject: IMPALA-3203: Part 2: per-core free lists in buffer pool .. IMPALA-3203: Part 2: per-core free lists in buffer pool Add per-core lists of clean pages and free pages to enable allocation of buffers without contention on shared locks in the common case. This is implemented with an additional layer of abstraction in "BufferAllocator", which tracks all memory (free buffers and clean pages) that is not in use but has not been released to the OS. The old BufferAllocator is renamed to SystemAllocator. See "Spilled Page Mgmt" and "MMap Allocator & Scalable Free Lists" in https://goo.gl/0zuy97 for a high-level summary of how this fits into the buffer pool design. The guts of the new code is BufferAllocator::AllocateInternal(), which progresses through several strategies for allocating memory. Misc changes: * Enforce upper limit on buffer size to reduce the number of free lists required. * Add additional allocation counters. * Slightly reorganise the MemTracker GC functions to use lambdas and clarify the order in which they should be called. Also adds a target memory value so that they don't need to free *all* of the memory in the system. * Fix an accounting bug in the buffer pool where it didn't evict dirty pages before reclaiming a clean page. Performance: We will need to validate the performance of the system under high query concurrency before this is used as part of query execution. The benchmark in Part 1 provided some evidence that this approach of a list per core should scale well to many cores. Testing: Added buffer-allocator-test to test the free list resizing algorithm directly. Added a test to buffer-pool-test to exercise the various new memory reclamation code paths that are now possible. Also run buffer-pool-test under two different faked-out NUMA setups - one with no NUMA and another with three NUMA nodes. buffer-pool-test, suballocator-test, and buffered-tuple-stream-v2-test provide some further basic coverage. Future system and unit tests will validate this further before it is used for query execution (see IMPALA-3200). Ran an initial version of IMPALA-4114, the ported BufferedBlockMgr tests, against this. The randomised stress test revealed some accounting bugs which are fixed. I'll post those tests as a follow-on patch. Change-Id: I612bd1cd0f0e87f7d8186e5bedd53a22f2d80832 --- M be/src/benchmarks/free-lists-benchmark.cc M be/src/common/init.cc M be/src/runtime/bufferpool/CMakeLists.txt A be/src/runtime/bufferpool/buffer-allocator-test.cc M be/src/runtime/bufferpool/buffer-allocator.cc M be/src/runtime/bufferpool/buffer-allocator.h M be/src/runtime/bufferpool/buffer-pool-counters.h M be/src/runtime/bufferpool/buffer-pool-internal.h M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/bufferpool/buffer-pool.cc M be/src/runtime/bufferpool/buffer-pool.h M be/src/runtime/bufferpool/free-list-test.cc M be/src/runtime/bufferpool/free-list.h M be/src/runtime/bufferpool/suballocator-test.cc M be/src/runtime/bufferpool/suballocator.h A be/src/runtime/bufferpool/system-allocator.cc A be/src/runtime/bufferpool/system-allocator.h M be/src/runtime/disk-io-mgr.cc M be/src/runtime/disk-io-mgr.h M be/src/runtime/exec-env.cc M be/src/runtime/mem-tracker.cc M be/src/runtime/mem-tracker.h A be/src/testutil/cpu-util.h A be/src/testutil/rand-util.h M be/src/util/cpu-info.cc M be/src/util/cpu-info.h 26 files changed, 1,513 insertions(+), 314 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/6414/14 -- To view, visit http://gerrit.cloudera.org:8080/6414 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I612bd1cd0f0e87f7d8186e5bedd53a22f2d80832 Gerrit-PatchSet: 14 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] Fix missing declaration of std::max in hdfs-avro-scanner-test
Henry Robinson has posted comments on this change. Change subject: Fix missing declaration of std::max in hdfs-avro-scanner-test .. Patch Set 1: Ping: who wants an easy review? :) I'm not sure why these issues show up on my KRPC branch (but not in trunk), but we should aim to include-what-we-use anyhow. -- To view, visit http://gerrit.cloudera.org:8080/6497 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id41c14e98163bcbb03675969e0a53754e92a4c14 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-3203: Part 2: per-core free lists in buffer pool
Tim Armstrong has posted comments on this change. Change subject: IMPALA-3203: Part 2: per-core free lists in buffer pool .. Patch Set 13: (15 comments) http://gerrit.cloudera.org:8080/#/c/6414/13/be/src/runtime/bufferpool/buffer-allocator.cc File be/src/runtime/bufferpool/buffer-allocator.cc: Line 35: static int64_t FREE_LIST_MAX_BYTES = 1024L * 1024L * 1024L; > how are these chosen? Pretty arbitrarily, I was just guestimating at what a reasonable upper bound to the amount of memory to hold onto was. Potentially we don't need these and could just rely on scavenging/maintenance to shrink the lists if they get too large. PS13, Line 94: BufferSize > PerSizeLists? Works for me. I struggled to come up with a good name since it's more of a grouping of data structures than an abstraction. http://gerrit.cloudera.org:8080/#/c/6414/13/be/src/runtime/bufferpool/buffer-allocator.h File be/src/runtime/bufferpool/buffer-allocator.h: Line 31: /// released to SystemAllocator. > buffers allocation lengths requested from this class must be a power of 2, Done Line 44: /// system error occurs (e.g. we can't allocate all of the required memory from the OS). > what about allocations beyond the limit? are they guaranteed to fail, or ar Done PS13, Line 50: len > required to be power of 2? Done PS13, Line 74: it > the buffer? Done PS13, Line 81: free > what does that mean? free back to the SystemAllocator? Tried to clarify. Not sure if the level of detail is appropriate for this comment. PS13, Line 108: 'min_buffer_len' > hard to imagine what this input means and how it affects the result without Done PS13, Line 115: 'len' > what is that? Done PS13, Line 122: The buffers are freed with 'system_allocator_' > why would memory that comes from 'buffer_bytes_remaining_' need to be freed That part was confusing. Reworded to make it clearer that there are two different strategies to come up with the memory. The code works out simpler to implement all the strategies in this method, but I guess it makes the name ScavengeBuffers less accurate. I'm not sure what a better alternative mightbe. PS13, Line 135: we support allocating > that can be allocated. i.e. i assume it's not just unsupported, it wouldn't Done PS13, Line 138: we support allocating. > same Done Line 159: static const int MAX_SCAVENGE_ATTEMPTS = 3; > what happens after this number of retries? elaborated in the comment. Line 164: }; > how do we (or will we) expose whatever needs to be exposed from this (and b Most of the perf counters are client-centric, so we will get a lot of info in the profiles for debugging query performance. We'll also get information through the MemTracker hierarchy (you can see used/reserved). We have DebugString() here too that dumps out the current state of the allocator when an allocation unexpectedly fails. I have some basic memory metrics (bytes in use, etc) in a follow-on patch that also switches to using mmap(). It would be easy enough to add more global metrics but it's unclear to me what would be actually useful for debugging. E.g. total #/bytes of allocations and total #/bytes of buffers released to the system might be marginally useful. http://gerrit.cloudera.org:8080/#/c/6414/13/be/src/runtime/bufferpool/buffer-pool-counters.h File be/src/runtime/bufferpool/buffer-pool-counters.h: Line 28: /// Amount of time spent trying to allocate a buffer. > there are multiple levels of "allocate" in the bufferpool subsystem. are th Done -- To view, visit http://gerrit.cloudera.org:8080/6414 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I612bd1cd0f0e87f7d8186e5bedd53a22f2d80832 Gerrit-PatchSet: 13 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Dan Hecht Gerrit-Reviewer: Taras Bobrovytsky Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[native-toolchain-CR] IMPALA-4226, IMPALA-4227: bump max threads, handle dwz compressed symbols
Lars Volker has submitted this change and it was merged. Change subject: IMPALA-4226, IMPALA-4227: bump max threads, handle dwz compressed symbols .. IMPALA-4226, IMPALA-4227: bump max threads, handle dwz compressed symbols IMPALA-4226: Increase the thread count limit in the minidump stack resolver. The old limit to 4096 is arbitrary and we are hitting it from time to time. To test this I ran minidump_stackwalk on a minidump with more than 4096 threads. IMPALA-4227: Add basic support for dwz dwarf extension The dwz tool [1] can be used to compress symbols that occur in multiple object files by moving them into a shared object file. It introduces new DWARF macros to reference to those symbols. Breakpad currently does not support those macros, which can lead to crashes. This change makes breakpad ignore these symbols. Impala's binaries don't share symbols so we are not losing functionality with this change. To test this I ran dump_syms on a binary compiled on CentOS 7, which uses DWZ for symbol compression. [1] https://sourceware.org/git/?p=dwz.git;a=summary Change-Id: I1bf83edd8cda037c31a842801ad1445f3fd4f71e --- M buildall.sh A source/breakpad/breakpad-88e5b2c8806bac3f2c80d2fe80094be5bd371601-patches/0001-Add-basic-support-for-dwz-dwarf-extension.patch A source/breakpad/breakpad-88e5b2c8806bac3f2c80d2fe80094be5bd371601-patches/0002-Increase-maximum-number-of-threads-for-minidump_stac.patch 3 files changed, 120 insertions(+), 1 deletion(-) Approvals: Lars Volker: Verified Tim Armstrong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/6461 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I1bf83edd8cda037c31a842801ad1445f3fd4f71e Gerrit-PatchSet: 3 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Lars Volker Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong
[native-toolchain-CR] IMPALA-4226, IMPALA-4227: bump max threads, handle dwz compressed symbols
Lars Volker has posted comments on this change. Change subject: IMPALA-4226, IMPALA-4227: bump max threads, handle dwz compressed symbols .. Patch Set 3: Verified+1 Verified in a private build. -- To view, visit http://gerrit.cloudera.org:8080/6461 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1bf83edd8cda037c31a842801ad1445f3fd4f71e Gerrit-PatchSet: 3 Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-Owner: Lars Volker Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
[native-toolchain-CR] Ported native-toolchain to work on ppc64le
Valencia Edna Serrao has posted comments on this change. Change subject: Ported native-toolchain to work on ppc64le .. Patch Set 1: (16 comments) > (5 comments) @Matt: Thanks for your comments. I've incorporated the modifications you asked for. > (11 comments) @Tim: Thanks for your comments. > I did a pass over most of it. It would be easier to review with > some cleanup to avoid duplicating code and make things consistent > with the style of the rest of the toolchain. I've aimed to eliminate the duplicated code in most of the places and align it to toolchain style with the help of your comments. But in some places, especially, using patching mechanism in crcutil and breakpad, I will require to add a conditional check for them in buildall.sh. I would like to know if there is any better way to do it. > I'm also reluctant to add patches here that aren't in the upstream > projects - we're not really equipped to review them. Yes...I understand your concern w.r.t patches which are not yet upstream. We are already communicating with the respective communities to get the patches upstreamed. http://gerrit.cloudera.org:8080/#/c/6468/1/buildall.sh File buildall.sh: PS1, Line 270: master > it would be better to use the same version as below, but just build_fake_pa Done. http://gerrit.cloudera.org:8080/#/c/6468/1/source/autoconf/build.sh File source/autoconf/build.sh: Line 31: if [[ "$(uname -p)" == "ppc64le" ]]; then > Can you add a variable to init.sh to hold the architecture instead of repea Done. I've added a new var 'ARCH_NAME' in init.sh that will be initialized to the underlying machine arch. Line 32: wrap ./configure \ > No need to wrap lines when they fit in 90 characters Ok. But this is as it is upstream. Line 35: else > The indentation of the if is wrong. Can you fix this here and elsewhere? Ok. Fixed. http://gerrit.cloudera.org:8080/#/c/6468/1/source/breakpad/build.sh File source/breakpad/build.sh: Line 35: wrap patch -p1 < $SOURCE_DIR/source/breakpad/breakpad-88e5b2c_ppc.patch > We already have a mechanism for including patches in the toolchain - see th Ok. I've been able to successfully use the native-toolchain patching mechanism to apply the ppc-patches for breakpad. However, this leads to conditional setting of BREAKPAD_VERSION in buildall.sh based on a arch check. http://gerrit.cloudera.org:8080/#/c/6468/1/source/crcutil/build.sh File source/crcutil/build.sh: Line 37: if [[ "$(uname -p)" == "ppc64le" ]]; then > Indentation Fixed. Line 42: wrap autoreconf -i; > Why not apply the patch before running autogen.sh above? The patch is applied on Makefile.am which is generated only after autogen.sh. Therefore, cannot apply the patch before autogen.sh. I used the native-toolchain patching mechanism here. It works fine for the "crc_interface_cc.patch", however, not for the "crc_makefile_am.patch", as the required file "Makefile.am" is not available at this point and will be generated later when the autogen.sh is executed. In this case, I might have to use my current approach without using the patching mechanism. Please let me know if there is any other better way to get the Makefile.am patch applied. http://gerrit.cloudera.org:8080/#/c/6468/1/source/crcutil/ppc-patches/crc_interface_cc.patch File source/crcutil/ppc-patches/crc_interface_cc.patch: Line 1: --- /home/test/crcutil-440ba7babeff77ffad992df3a10c767f184e946e/examples/interface.cc 2014-05-23 09:31:26.0 +0530 > I don't really understand what this patch does and we don't know enough abo Since this code is part of the examples section, I have just worked around the compilation error. I am yet to fix it appropriately. Once done I’ll surely push it upstream. http://gerrit.cloudera.org:8080/#/c/6468/1/source/cyrus-sasl/build.sh File source/cyrus-sasl/build.sh: Line 51: CFLAGS="$CFLAGS -fPIC -DPIC" CXXFLAGS="$CXXFLAGS -fPIC -DPIC" wrap ./configure \ > Why not just put the additional flag into CONFIGURE_FLAGS above? Agreed. I've done the change. http://gerrit.cloudera.org:8080/#/c/6468/1/source/glog/build.sh File source/glog/build.sh: Line 37: --build=powerpc64le-unknown-linux-gnu \ > The command is mostly the same. Why not factor out the different flag into Yes. I've done the change. http://gerrit.cloudera.org:8080/#/c/6468/1/source/kudu/build.sh File source/kudu/build.sh: Line 68: > Can you remove the build changes for now since it sounds like this is still Ok. Done PS1, Line 70: git clone https://github.com/ibmsoe/kudu > The Kudu changes will be merged into upstream Kudu master, right? Yes. Right. PS1, Line 121: echo "Installing gcc-4.9.3 to build kudu src code on ppc" : source $SOURCE_DIR/source/kudu/setup_gcc493.sh > why do you need a different version of gcc to build kudu but not for other GCC_VERSION in init.sh is set to 4.9.2. This gcc version works perfect for all
[Impala-ASF-CR] IMPALA-4701: make distcc work reliably with clang
Lars Volker has posted comments on this change. Change subject: IMPALA-4701: make distcc work reliably with clang .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/6493/5/bin/distcc/distcc.sh File bin/distcc/distcc.sh: Line 19: Would it make sense to add "set -euo pipefail" here? -- To view, visit http://gerrit.cloudera.org:8080/6493 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I199b245fb14b6c3484b66339a7d4b37d74755af7 Gerrit-PatchSet: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4758: (2/2) Impala-side changes to build with latest gutil
Impala Public Jenkins has posted comments on this change. Change subject: IMPALA-4758: (2/2) Impala-side changes to build with latest gutil .. Patch Set 8: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/5688 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4ac21d7d6401f21fcdfdd1132b8f322bfba4bb80 Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Henry Robinson Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Jim Apple Gerrit-Reviewer: Tim Armstrong Gerrit-HasComments: No
