Code review comment for lp:~sergei.glushchenko/percona-xtrabackup/xb-tools

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

This is a partial review only, I will continue later.

    Please create and link a blueprint for this work.

    Is it possible to merge the tools InnoDB patch with some existing
    patch? I guess for that some more effort would be required: to
    uncomment but conditionally skip the commented
    innobase_rec_to_mysql(), trx_i_s_cache_init() etc. calls. I don't
    know if this additional effort is justifiable or not.

    Instead of open_or_create_log_file_ex() perhaps better to have
    just open_log_file() that always sets the proposed log file from
    the actual file size? In general I don't like the _ex naming
    convention, because as time passes it is never apparent what is
    meant by it.

    What is the purpose of rec_get_offsets_old()? Its implementation
    does not have a header comment, and the one next to its
    declaration does not tell why "new" function is not good enough.

    Why there are separate rules for the utility functions in XB? I
    think we should just link them in the relevant xb configurations
    by default. If they are needed then the Makefile diff at lines
    591--599 contains $(CFLAGS), $(CXXFLAGS), $(CCFLAGS). I think
    only the first is correct, and then the rules can be merged.

    Makefile "utils" target should be marked as phony and IMHO it
    should be included in the default build.

    Can the INNODBOBJS definition for utils be reused from some other
    target instead of copy-paste?

    Line 677: please define MLOG_LSN conditionally with #ifndef
    MLOG_LSN

    The ib_log_rec_*, ib_log_sys_* structs and functions need
    comments. This was my work, please ping me on IRC and we will
    coordinate.

    The #if 0 blocks in fill_parse_buffer should go away. Likewise in
    ib_log_sys_g et_next_rec().

    The untested log record types still need testing. That's a non
    trivial task, need coordination with QA.

review: Needs Fixing

« Back to merge proposal