Skip to content

CODE_REVIEW_GUIDE.md

Latest commit

 

History

History
199 lines (167 loc) · 14.6 KB

CODE_REVIEW_GUIDE.md

File metadata and controls

199 lines (167 loc) · 14.6 KB
Raw

Code Review Guide

For human reviewers and AI review agents. This repo is a benchmark. Its output (tpmC, latency percentiles, retry rates) is compared across database builds and over time by performance pipelines. The most damaging bug here doesn't crash; it silently changes the numbers. Review with that in mind.

1. Invariants (must hold)

Workload semantics

  1. 10 terminals per warehouse, each pinned to one (w_id, d_id). (BenchmarkModule.createTerminals)
  2. NewOrder, Payment and OrderStatus pick a random district 1..10. StockLevel uses the terminal's district. (Worker.executeWork)
  3. 1% of NewOrders roll back through UserAbortException and count as successes (status USER_ABORTED). Nothing else may throw UserAbortException.
  4. NewOrder writes OORDER before NEW_ORDER and ORDER_LINE (FK order).
  5. StockLevel runs in autocommit. Every other transaction runs with autoCommit=false, committed by Worker.executeWork (or earlier inside the procedure).
  6. NURand constants and ranges match TPC-C 2.1.6 (TPCCUtil).

Measurement integrity

  1. A terminal cycle is recorded only if the state is MEASURE after its think-time sleep, and was WARMUP/MEASURE when the work was fetched (Worker.run, the PART 4 switch). Don't move these checks without understanding the window shift (see METRICS_AND_RESULTS.md).
  2. tpmC = recorded NewOrder SUCCESS + USER_ABORTED × 60 / runtime. Failed attempts never count.
  3. Latency units: samples are µs; tables and JSON are ms (LatencyMetricsUtil divides by 1000); CSV is raw µs and ns. Don't mix them.
  4. Keying and think sleeps stay outside the "operation" latency, and the connection is returned to the pool before the think sleep.
  5. The transaction list order is NewOrder, Payment, OrderStatus, Delivery, StockLevel. DBWorkload hard-codes ids 1..5 to those names and sizes arrays at 5.
  6. JSON field names (TpccRunResults) are consumed downstream. Don't rename or remove fields; add new ones instead.

Schema / DDL

  1. Every schema change is made for both dbtypes (yugabyte HASH keys vs postgres) and in both schema managers (DefaultSchemaManager and GeoPartitionedSchemaManager).
  2. Table.getInsertDml inserts by column position. Loader setters must follow TPCCTableSchemas column order exactly.
  3. FKs are added after load as NOT VALID, and dropped before being added (idempotent).

Concurrency

  1. Procedure instances are per worker (they hold PreparedStatement fields). Never share one across threads or make those fields static.
  2. Per-statement histograms are static and shared, so they must stay ConcurrentHistogram.
  3. Use the worker's own Random gen (or ThreadLocalRandom) in hot paths. Don't add new shared Random/synchronized objects to the per-transaction path.

2. Review checklist

Hot-path performance (NewOrder and Payment are 88% of the mix)

  • Does the change add a round trip (a new statement, a separate SELECT before an UPDATE, splitting a multi-row statement)? Each one is a network hop in a distributed DB and lowers tpmC. Prefer UPDATE ... RETURNING, IN (...) lists, multi-row VALUES, or server-side procedures.
  • Does it add locking (FOR UPDATE, FOR SHARE, FOR KEY SHARE)? These were removed or reverted on purpose (#60, #102, #131). Require benchmark numbers.
  • Does it add a write to a hot row (WAREHOUSE, DISTRICT) or lengthen the window between reading and writing it? Expect more 40001 retries.
  • Per-transaction logging at INFO/WARN? Not allowed. Use DEBUG/TRACE and guard expensive message construction with isTraceEnabled().
  • Is there new allocation per transaction that scales with terminals (thousands of threads)?

Correctness of the workload

  • Transaction logic still matches TPCC_WORKLOAD.md (or the doc is updated and the deviation is intentional).
  • Bind parameter indices match the SQL placeholders (see known issue K1, a real instance of this bug).
  • Code that pairs rows from two result sets doesn't assume they come back in the same order (K4).
  • Data generated by Loader still satisfies spec cardinalities (§5 of the workload doc).

Retries and errors

  • Failures are not silently counted as successes, and successes are not double-counted across retry attempts.
  • New exception types go to the right status. UserAbortException is only for the expected NewOrder rollback.
  • Rollback happens before a retry, and rollback errors are tolerated (the node may be down). Today only the SQLException path rolls back (K20). Don't widen that gap.
  • Logging: expected contention errors (40001 etc.) stay at DEBUG. Other errors log WARN for intermediate attempts and ERROR for the final one (#154). No full stack traces on every retry. If you need a trace, pass the exception to the logger instead of concatenating getStackTrace() (K22).

Measurement

  • Timestamps (System.nanoTime()) are still captured at the same boundaries: begin, fetched, keyed, executed, thought.
  • Any new metric is reported consistently on stdout and in JSON, and handled by mergeJsonResults (or the merge limitation is documented).
  • Ms vs µs conversions are correct (invariant 9).

Schema, config, CLI

  • Both dbtypes and both schema managers updated (invariant 13).
  • New XML keys: getter in ConfigFileOptions, default in WorkloadConfiguration, wiring in DBWorkload, sample value in both config/workload_all*.xml, and CONFIGURATION.md updated.
  • New CLI flags follow the existing --name=value style. Boolean modes use =true.
  • Defaults don't change silently. Perf pipelines run with existing flags, so a changed default changes historical comparability. Call it out in the PR description.

Build and dependencies

  • A dependency bump updates download-deps.sh (used by ant resolve), ivy.xml and pom.xml together.
  • Source stays compatible with the JDKs used by the perf clients (JDK 8+ syntax; the code compiles on JDK 17).
  • No real credentials in config/*.xml (workload_all_pg.xml ships a placeholder password).

3. How to validate a change

There is no automated test suite: no unit tests in the build, and no CI in this repo. Validate manually:

  1. Compile: ant build (or the javac fallback in RUNNING.md).
  2. Small end-to-end run against a local single-node YugabyteDB (e.g. yugabyted start) and, if you touched dbtype-dependent code, PostgreSQL: 
    ./tpccbenchmark --create=true
./tpccbenchmark --load=true --warehouses=2
# quick functional smoke test: edit a copy of the config to set useKeyingTime/useThinkTime=false and runtime=60
./tpccbenchmark -c /tmp/smoke.xml --execute=true --warehouses=2 --vv
    Check: no unexpected ERROR logs; the retry table looks sane; every transaction type has a non-zero count.
  3. Data consistency checks after the run. These are TPC-C consistency conditions; every query should return 0 rows: 
    -- CC1: W_YTD = sum(D_YTD)
SELECT w_id FROM warehouse w
 WHERE w_ytd <> (SELECT sum(d_ytd) FROM district WHERE d_w_id = w.w_id);
-- CC2: D_NEXT_O_ID - 1 = max(O_ID) = max(NO_O_ID)   (NEW_ORDER part may be empty after heavy Delivery)
SELECT d_w_id, d_id FROM district d
 WHERE d_next_o_id - 1 <> (SELECT max(o_id) FROM oorder WHERE o_w_id = d.d_w_id AND o_d_id = d.d_id);
-- CC3: NEW_ORDER ids are contiguous per district
SELECT no_w_id, no_d_id FROM new_order GROUP BY no_w_id, no_d_id
HAVING max(no_o_id) - min(no_o_id) + 1 <> count(*);
-- CC4: sum(O_OL_CNT) = count(ORDER_LINE) per district
SELECT o_w_id, o_d_id FROM oorder o GROUP BY o_w_id, o_d_id
HAVING sum(o_ol_cnt) <> (SELECT count(*) FROM order_line
                          WHERE ol_w_id = o.o_w_id AND ol_d_id = o.o_d_id);
  4. Performance-sensitive changes: run baseline and change on the same cluster, data and flags, with spec keying/think times, long enough to reach steady state. Compare tpmC, efficiency, NewOrder p99, Retry #0 %, and per-SQL stats (trackPerSQLStmtLatencies=true). Attach the numbers to the PR.
  5. NewOrder logic changes: NewOrder.test() asserts exact STOCK/DISTRICT/OORDER/ ORDER_LINE effects for crafted orders. It is reachable only via BenchmarkModule.test() (not from the CLI), so wire it up temporarily or port the cases.

Known issues & tech debt

Found by reading the code at commit 53a41dd. Each one is verified against the source unless it is marked suspected. Fixing one may change benchmark numbers, so give it its own PR with before/after results.

# Location Issue Impact
K1 NewOrder.updateStock (only when useStoredProcedures=false) Binds whId to S_I_ID and itemId to S_W_ID (the SQL is WHERE S_I_ID = ? AND S_W_ID = ?). Also S_YTD = S_YTD + ? and S_REMOTE_CNT = S_REMOTE_CNT + ? are bound to the new absolute values, not increments. The non-procedure path updates the wrong rows (or none) and inflates counters. Results with useStoredProcedures=false are not valid.
K2 Delivery.run loop d_id <= terminalDistrictUpperID together with Worker.executeWork Delivery processes districts 1..terminalDistrict instead of all 10. Delivery does about 55% of the spec's work; NEW_ORDER drains unevenly (district 1 most, district 10 least).
K3 GeoPartitionedSchemaManager.createSqlProcedures Loops partition < numPartitions, so the last partition gets no updatestockN_<p>, and the routing updatestockN has no branch for its warehouses. Suspected: stock updates there are silently skipped. Geo runs under-report write work in the last partition.
K4 NewOrder.getItemsAndStock Iterates the ITEM and STOCK IN (...) result sets in lockstep, assuming the same row order. Item price/data comes from the ITEM row, the key from the STOCK row. The asserts that would catch a mismatch are disabled. Possible wrong ol_amount/brand-generic attribution. Throughput is unaffected.
K5 Loader.load* Every exception is caught, logged (Failed to load data for TPC-C) and swallowed; the load still reports success. Partially loaded databases go unnoticed. Always check row counts.
K6 Worker.doWork outer catch (SQLException) A failure in getConnection()/close() (e.g. a Hikari timeout) becomes a RuntimeException, then the uncaught handler calls System.exit(-1). One unreachable node or pool starvation kills the whole run instead of being counted as a failure. A fix is proposed in PR #162 (and #161 for pool start-up).
K7 CommandLineOptions --histograms; Results.txn* The option takes no argument but is read as =true, so it can never be enabled. The histograms it would print are never incremented. Dead feature.
K8 StockLevel printLatencyStats reports stockGetCountStockSQL's histogram, but the callable records into stockGetCountStockFuncLatency. The GetCountStock per-statement line is always empty.
K9 DBWorkload.PrintLatencies The JSON FailureLatencies "All" row passes the success connection latencies. Wrong connectionAcqLatency in that row.
K10 DBWorkload (transactionTypes map, combineListsAcrossTransactions, arrays of size 5) Hard-codes 5 transaction types in a fixed order. Reordering or removing transactions in the XML mislabels or breaks the reports.
K11 Procedure.getPreparedStatement Asserts membership in a map that is always empty (fields are InstrumentedSQLStmt, not SQLStmt). Can't run with -ea. Other stale asserts exist too.
K12 Worker.doWork One empty catch (Throwable) wraps both SET yb_enable_expression_pushdown and setAutoCommit(false) (added in #148). If the SET fails (e.g. a server without that setting), setAutoCommit(false) is skipped. Every non-StockLevel transaction then runs in autocommit (non-atomic), and its commit/rollback are skipped.
K13 DBWorkload.mergeResults (CSV merge) Computes a row's end time as start + connection latency, not operation latency. Slight over-count at the window edge. Prefer the JSON merge.
K14 InstrumentedPreparedStatement.trackLatencies Defaults to true when <trackPerSQLStmtLatencies> is absent (the sample XML sets it false). Surprising extra timing overhead and output for configs that omit it.
K15 Payment.run, OrderStatus.run, Delivery.run Call conn.commit() (Delivery once per district), and then Worker.executeWork commits again. Harmless (the second commit is a no-op), but confusing.
K16 TPCCUtil NURand C constants are hard-coded, not drawn per run. Minor spec deviation.
K17 Stale artifacts build.xml execute/dialects-export targets pass a removed -b flag. setup_clients.sh pins release 1.9. The workload XML comment says "set retries to 0" next to a value of 2. log4j.properties lists removed benchmarks. Confusion only.
K18 Worker static wrkld/wrkldState; TPCCTableSchemas static cache Assumes one benchmark and one dbtype per JVM. Blocks multi-benchmark or multi-dbtype use in one process.
K19 BenchmarkModule.createDataSource Always sets dataSourceClassName, so Hikari ignores the config.setJdbcUrl(<jdbcURL>) call (it logs "using dataSourceClassName and ignoring jdbcUrl"). <jdbcURL> options never reach the execute pools. It only affects create/load/FK/procedure connections.
K20 Worker.doWork catch (Error) / catch (Exception) These set RETRY without rolling back, so the NewOrder retry runs on the same connection with the failed transaction still open. Also, an SQLException with a null SQLState stays UNKNOWN and is not retried. Retries after non-SQL failures may fail again ("current transaction is aborted") or run inside the old transaction.
K21 TPCCUtil.randomStr Returns n-1 characters (astring(n-1, n-1)). All generated alpha strings are one character shorter than the spec (i_data 22–49, s_dist_xx 23, ...). Slightly smaller rows.
K22 Worker.doWork final-attempt logs Concatenate ex.getStackTrace(), which prints [Ljava.lang.StackTraceElement;@..., not a trace. Final-failure logs lose the stack trace.