yugabyte / tpcc

Repo to run TPCC benchmarks against YugabyteDB
Other
24 stars 21 forks source link

Add per sql-statement metrics for various transactions in TPCC #79

Closed amitanandaiyer closed 3 years ago

robertsami commented 3 years ago

@amitanandaiyer nice! this seems great to have, but wondering a couple things:

  1. can we take an approach that avoids code duplication?
  2. can we support turning this on/off? maybe by txn type or by statement type?

one thing to consider that may be a bit cleaner is introducing something like the following:

class StatementExecutor {
  StatementExecutor(Connection conn);

  int executeUpdate(PreparedStatement stmt);

  ResultSet executeQuery(PreparedStatement stmt);
// ... etc.
}

this way we can inject custom functionality, such as tracking timings, as needed. note @psudheer21 that this kind of construct would also more cleanly deal with the retry logic introduced in TPCCLoader recently

psudheer21 commented 3 years ago

@amitanandaiyer @robertsami Would it be better to Extend the "PreparedStatement" class to our version that includes this functionality? That can take as arguments the SQL string, the Histogram pointer, Txn type, etc..

All it would do for methods like "executeUpdate", "executeQuery etc. is call the Base class' method and calculate latency..

That way most of the code will remain same. And we will have to only change the getPreparedStatement() method. Let me know what you think.

robertsami commented 3 years ago

@psudheer21 i think your proposal is a more expedient solution. i prefer composition over inheritance in such cases, as in my experience extending third party library classes can be fragile, but not going to argue strongly one way or the other!

amitanandaiyer commented 3 years ago

metrics_disabled.txt metrics_enabled.txt