[TDB-20] With checkpoint disabled, closing FT file still rotates header #383

Jun-Yuan · 2017-09-04T11:17:47Z

Updated: fixed the format again.
new jenkins test: http://jenkins.percona.com/view/TokuDB/job/PerconaFT-param/107/

Updated: fixed the format.
new Jenkins test: http://jenkins.percona.com/view/TokuDB/job/PerconaFT-param/106

Jenkins: http://jenkins.percona.com/view/TokuDB/job/PerconaFT-param/105/

    This is an attempt based on @BohuTang's patch
    https://github.com/percona/PerconaFT/pull/331

    in particular, the ctest is all his credits.

    Summary:
    1) The issue is real.
    2) @Bohu's patch works for mediating the issue but somewhat introduces
    a ft leak that the ft with ref=0 may be left until next open/close or
    checkpoint.
    3) If ft_close should be held off upon a backup, then it simply means
    a backup should hold a ref to the ft and responsible for closing ft up
    if it is the last one to hold it, like any other possible referencer.
    including the checkpoint, the txn and ft handlers.
    4) mtr test contributed by @BohuTang still passes

laurynas-biveinis

Formatting comments only

laurynas-biveinis · 2017-09-05T06:36:30Z

ft/cachetable/cachetable-internal.h

    KIBBUTZ client_kibbutz; // pool of worker threads and jobs to do asynchronously for the client.
    KIBBUTZ ct_kibbutz; // pool of worker threads and jobs to do asynchronously for the cachetable
    KIBBUTZ checkpointing_kibbutz; // small pool for checkpointing cloned pairs
+    //bool in_backup; // we are in back up or NOT, default is false


Please no commented-out code?

laurynas-biveinis · 2017-09-05T06:37:15Z

ft/ft.cc


+
+// maps to cf->note_pin_by_backup
+//Must be protected by ydb lock.


Spaces after //

laurynas-biveinis · 2017-09-05T06:37:41Z

ft/ft.cc

+// Requires: the reflock is held.
+static void unpin_by_backup_callback(FT ft, void *extra) {
+    invariant(extra == NULL);
+    invariant(ft->num_backups>0);


Spaces around >

laurynas-biveinis · 2017-09-05T06:37:50Z

ft/ft.cc

+static void unpin_by_backup_callback(FT ft, void *extra) {
+    invariant(extra == NULL);
+    invariant(ft->num_backups>0);
+    ft->num_backups --;


No space before the operator

laurynas-biveinis · 2017-09-05T06:38:02Z

ft/ft.cc

+}
+
+// maps to cf->note_unpin_by_backup
+//Must be protected by ydb lock.


Spaces after //

laurynas-biveinis · 2017-09-05T06:38:29Z

ft/tests/ft-in-backup-test.cc

+This file is part of PerconaFT.
+
+
+Copyright (c) 2006, 2015, Percona and/or its affiliates. All rights reserved.


Still missed s/2015/2017

laurynas-biveinis · 2017-09-05T06:38:37Z

ft/tests/ft-in-backup-test.cc

+    along with PerconaFT.  If not, see <http://www.gnu.org/licenses/>.
+======= */
+
+#ident "Copyright (c) 2006, 2015, Percona and/or its affiliates. All rights reserved."


omg...how did you spot it...hawkeyes

laurynas-biveinis · 2017-09-05T06:39:58Z

ft/tests/ft-in-backup-test.cc

+
+    toku_cachetable_create(&ct, 0, ZERO_LSN, nullptr);
+    // TEST1 : for normal
+    r = toku_open_ft_handle(fname, 1, &ft, 1<<12, 1<<9, TOKU_DEFAULT_COMPRESSION_METHOD, ct, null_txn, toku_builtin_compare_fun);


This and similar lines below are probably too long. A new file can be clang-formatted without much damage I guess

Jun-Yuan · 2017-09-15T02:55:24Z

Thanks. Please review my follow-up

laurynas-biveinis · 2017-09-18T05:52:03Z

ft/ft.cc

+    FT ft = (FT) header_v;
+    toku_ft_grab_reflock(ft);
+    assert(toku_ft_needed_unlocked(ft));
+    ft->num_backups ++;


No space before the postincrement operator

laurynas-biveinis · 2017-09-18T05:53:06Z

ft/ft.cc

                                ft_note_pin_by_checkpoint,
-                                ft_note_unpin_by_checkpoint);
+                                ft_note_unpin_by_checkpoint,
+				ft_note_pin_by_backup,


Indentation must be wrong either in the added lines, either in the lines above (in which I'm not sure it should be fixed)

laurynas-biveinis · 2017-09-18T05:53:19Z

ft/ft.cc

                                ft_note_pin_by_checkpoint,
-                                ft_note_unpin_by_checkpoint);
+                                ft_note_unpin_by_checkpoint,
+				ft_note_pin_by_backup,


Updated again. Thanks!

laurynas-biveinis · 2017-09-18T05:53:31Z

ft/tests/cachetable-pin-checkpoint.cc

        &dummy_note_pin,
-        &dummy_note_unpin
+        &dummy_note_unpin,
+	&dummy_note_pin,


laurynas-biveinis · 2017-09-18T05:53:37Z

ft/tests/cachetable-put-checkpoint.cc

        &dummy_note_pin,
-        &dummy_note_unpin
+        &dummy_note_unpin,
+	&dummy_note_pin,


laurynas-biveinis · 2017-09-18T05:53:44Z

ft/tests/cachetable-simple-close.cc

        &dummy_note_pin,
-        &dummy_note_unpin
+        &dummy_note_unpin,
+	&dummy_note_pin,


laurynas-biveinis · 2017-09-18T05:53:51Z

ft/tests/cachetable-test.h

                               &dummy_end,
                               &dummy_note_pin,
+                               &dummy_note_unpin,
+			       &dummy_note_pin,


@BohuTANG

…der. This is an attempt based on @BohuTANG's patch percona#331 in particular, the ctest is all his credits. Summary: 1) The issue is real. 2) @bohu's patch works for mediating the issue but somewhat introduces a ft leak that the ft with ref=0 may be left until next open/close or checkpoint. 3) If ft_close should be held off upon a backup, then it simply means a backup should hold a ref to the ft and responsible for closing ft up if it is the last one to hold it, like any other possible referencer. including the checkpoint, the txn and ft handlers. 4) mtr test contributed by @BohuTANG still passes

laurynas-biveinis suggested changes Sep 5, 2017

View reviewed changes

Jun-Yuan force-pushed the TDB-20 branch from b4ea5f7 to abcf8bb Compare September 14, 2017 03:15

laurynas-biveinis suggested changes Sep 18, 2017

View reviewed changes

Jun-Yuan force-pushed the TDB-20 branch from abcf8bb to 18649a3 Compare September 19, 2017 19:17



		// maps to cf->note_pin_by_backup
		//Must be protected by ydb lock.

		This file is part of PerconaFT.


		Copyright (c) 2006, 2015, Percona and/or its affiliates. All rights reserved.

[TDB-20] With checkpoint disabled, closing FT file still rotates header #383

Are you sure you want to change the base?

[TDB-20] With checkpoint disabled, closing FT file still rotates header #383

Uh oh!

Conversation

Jun-Yuan commented Sep 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laurynas-biveinis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jun-Yuan commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Jun-Yuan commented Sep 4, 2017 •

edited

Loading