search

killbill / killbill-plugin-framework-ruby

Framework to write Kill Bill plugins in Ruby
http://killbill.io
8 stars 11 forks source link

The max_nb_records field in the pagination causes DB performance issue #67

Closed Ecsiral closed 5 years ago

Ecsiral commented 5 years ago

What is the purpose of having the max_nb_records field in the pagination?

The counting query will be performed for every single search request, while it scans the entire responses table on a boolean column.

Can we remove the max_nb_records field? Or can we add an option indicating if clients want the field or not (by default)? Or a delayed-running when the client calls the get method of the field?

The code locates at https://github.com/killbill/killbill-plugin-framework-ruby/blob/master/lib/killbill/helpers/active_merchant/active_record/models/response.rb#L201

Ecsiral commented 5 years ago

I suggested a quick fix #68 . But 1) we still need an overall reconsideration on the field, all plugins have to override the method when they enable the search feature due to the performance issue, 2) the issue is in java plugins as well.

pierre commented 5 years ago

What is the purpose of having the max_nb_records field in the pagination?

It's the total number of records in the complete data set, regardless of paging and filtering (see https://github.com/killbill/killbill-api/blob/master/src/main/java/org/killbill/billing/util/entity/Pagination.java#L44-L51). Equivalent to count(*). It is used to display pagination legends in Kaui (e.g. Showing 1 to 50 of 10,654 entries).

The counting query will be performed for every single search request, while it scans the entire responses table on a boolean column.

Note the Javadoc: it doesn't have to be accurate. In fact, in Kill Bill core, it's is accurate only up to 20k entries. Above, Kaui would display Showing 1 to 50 of 20,000+ entries.

Can we remove the max_nb_records field? Or can we add an option indicating if clients want the field or not (by default)? Or a delayed-running when the client calls the get method of the field?

Such modifications would require API changes, and, if accepted, would only be available in 0.22.x. You would first need to upgrade from 0.18.x to 0.20.x, before being able to upgrade to 0.22.x.

I suggested a quick fix #68 . But 1) we still need an overall reconsideration on the field, all plugins have to override the method when they enable the search feature due to the performance issue

It is indeed expected that plugins override these search methods and/or update indexes as needed. The base framework provides a basic working implementation again the generic ActiveMerchant API but in practice, plugins are all very different.

Note also how young the Orbital plugin is (<100 commits): it was autogenerated and pretty much put in production as-is, without much needed tweaking, performance testing, etc.

2) the issue is in java plugins as well.

Performance with Java plugins are much better in general: note how fast the /accounts page in Kaui loads with tens of millions of accounts for instance (this code isn't running in a plugin, but would be very similar if it did).

I am going to close this issue as "working as expected", but feel free to reopen if you disagree. My recommendation would be:

  1. Ideally, rewrite it in Java
  2. If this isn't practical, load test and tweak the implementation as needed (happy to provide more pointers if needed)