Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16581 )
Change subject: KUDU-3195: flush when any DMS in the tablet is older than the time threshold ...................................................................... Patch Set 6: (14 comments) http://gerrit.cloudera.org:8080/#/c/16581/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16581/5//COMMIT_MSG@9 PS5, Line 9: least 2 minutes > nit: maybe, it's worth pointing that it's controlled by the --flush_thresho Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.h File src/kudu/tablet/delta_tracker.h: http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.h@238 PS5, Line 238: a D > nit: drop Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.h@239 PS5, Line 239: . Otherwise, returns false and doesn't upda > nit: maybe, it's worth mentioning what's the expectation on filling in the Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.h@239 PS5, Line 239: ich it was create > nit: maybe, name it as GetDMSInfo()/GetDeltaMemStoreInfo() or alike, pointi Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.cc File src/kudu/tablet/delta_tracker.cc: http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.cc@867 PS5, Line 867: // Check dms_exists_ first to avoid unnecessary contention on : // component_lock_. We ne > nit: do we expect high contention on component_lock_? If so, then maybe tr Good point. We do this in other methods, so we might as well do so here. http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/delta_tracker.cc@873 PS5, Line 873: *size_bytes = dms_->EstimateSize(); : *creation_time = dms_->c > Is it really necessary to bother filling in these if DMS isn't there? Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/memrowset.h@381 PS5, Line 381: return false; : } > nit: why to bother filling in these if returning 'false' anyways? Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/rowset.h File src/kudu/tablet/rowset.h: http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/rowset.h@429 PS5, Line 429: virtual Status DebugDump(std::vector<std::string> *lines = nullptr) override; > warning: use nullptr [modernize-use-nullptr] Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/rowset.h@436 PS5, Line 436: return nullptr; > warning: use nullptr [modernize-use-nullptr] Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/tablet.cc@2234 PS5, Line 2234: // To facilitate memory-based flushing when under memory pressure, we : // define a score that's part memory and part WAL retention bytes. : double score = dms_size_bytes * mem_weight + replay_size_bytes * (100 - mem_weight); > Should it be moved into a method/function? I guess it might be re-used in It is not reused anywhere right now, and I actually question its value. I'm not sure the assessment on KUDU-2238 is complete, given they waited hours for a flush to happen. For now, I'm leaving it as is. http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/tablet_replica_mm_ops.cc File src/kudu/tablet/tablet_replica_mm_ops.cc: http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tablet/tablet_replica_mm_ops.cc@268 PS5, Line 268: now - earliest_dms_time).ToMillisec > What if FindBestDMSToFlush() didn't find any DMS? Does this stay semantica Negative MonoDeltas are valid, though I'll update this to be more conservative with the MonoTime::Max() case. http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tserver/tablet_server-test.cc@4335 PS5, Line 4335: KUDU-3195 > KUDU-3195 ? Done http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tserver/tablet_server-test.cc@4356 PS5, Line 4356: > nit: maybe, using base::NumCPUs() would look a bit more natural? Actually this is fairly unimportant; I'll just stick with a single MM thread. http://gerrit.cloudera.org:8080/#/c/16581/5/src/kudu/tserver/tablet_server-test.cc@4362 PS5, Line 4362: r > Just to make sure I understand: does this test create enough updates/deltas Yep, there are 100 rowsets, and therefore 100 DMSs to flush. Before, we would wait 1 second between scheduling each one, so with a single thread, that would take 100s. -- To view, visit http://gerrit.cloudera.org:8080/16581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id05202bf6a4685f4d79db11ef8ebb0f91f6316b4 Gerrit-Change-Number: 16581 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 14 Oct 2020 00:00:08 +0000 Gerrit-HasComments: Yes
