Code review comment for lp:~vlad-lesin/percona-xtrabackup/2.1-apply-archived-logs-innodb5.6

Vlad,

    - I don't think the XtraBackup part should pull the entire server
      implementation of log archiving. It looks like only a minor part
      of it should be applied to InnoDB 5.6 code base to be able to
      parse and apply archived logs? As in, do we really need
      fil_space_contains_node(), log_sys->archive_buf initialization,
      changes to log_checkpoint_set_nth_group_info(), etc.?

    - can you clarify the following change? I.e. under what
      circumstances exactly can a tablespace exist when we are trying
      to replay MLOG_FILE_CREATE from the archived logs?

+@@ -3325,9 +3341,13 @@
+ path = fil_make_ibd_name(tablename, false);
+ }
+
++ /* The files can already exist in the case of archived logs applying */
+ file = os_file_create(
+ innodb_file_data_key, path,
+- OS_FILE_CREATE | OS_FILE_ON_ERROR_NO_EXIT,
++ (srv_archive_recovery ?
++ OS_FILE_OVERWRITE :
++ OS_FILE_CREATE) |
++ OS_FILE_ON_ERROR_NO_EXIT,
+ OS_FILE_NORMAL,
+ OS_DATA_FILE,
+ &ret);

    - s/alligned to the up of log file block/aligned up to the log block size/

    - please clarify why the following change is needed. As I
      understand, we don't start the FTS optimize thread in
      XtraBackup. That code is also missing block braces and is using
      spaces instead of tabs for indentation:

+@@ -3032,7 +3032,8 @@
+
+ ib_logf(IB_LOG_LEVEL_INFO, "FTS optimize thread exiting.");
+
+- os_event_set(exit_event);
++ if (exit_event)
++ os_event_set(exit_event);
+
+ /* We count the number of threads in os_thread_exit(). A created
+ thread should always use that to exit and not use return() to exit. */
+

    - please use LSN_PF instead of "%llu"

    - in recv_recovery_from_archive_start() we ignore the first_log_no
      argument and use xtrabackup_arch_first_file_lsn instead. wouldn't
      it be better to pass xtrabackup_arch_first_file_lsn as the
      first_log_no argument to recv_recovery_from_archive_start() and
      get rid of min/max_arch_log_no in
      innobase_start_or_create_for_mysql()?

    - in xb_data_files_init() the patch replaces min/max_flushed_lsn
      declarations with min/max_arch_log_no. Which are not really
      used. but then the call still calls open_or_create_data_files()
      with the (now non-existing) flushed_lsn variables?

    - s/xtrabackup: Note: opening archived log directory %s was
      failed/xtrabackup: error: cannot open archived log directory %s/

    - replace the strstr() call in xtrabackup_arch_search_files() with
      strncmp()

    - xtrabackup_arch_search_files() is declared to return bool, but
      returns xtrabackup_arch_first_file_lsn?

    - we tend to use the InnoDB code style in xtrabackup.cc. In
      particular, the opening brace must be on the same line as the
      'if' statement. And braces should be used even for
      single-statement 'if' blocks;

    - s/trunsactions/transactions/

    - s/undid/rolled back/

    - the check for Percona Server 5.6 doesn't work (try starting it
      with an upstream 5.6 server). The reason is that the syntax for
      string equality comparison is "val1 = val2", not "val1=val2"

    - the test case doesn't work for me (fails with "table 'test.t1'
      doesn't exist" when run against PS 5.6 with log archiving enabled)