Code review comment for lp:~oontvoo/akiban-server/bug_drop_fulltext_index

Revision history for this message
Nathan Williams (nwilliams) wrote :

> That is not a *little* optimisation. The current getIndex()
> would cause a bunch of stuff to be done including building
> a list of rowTypes and computing plans to lookup related
> rows. Why would we want to do all that just to delete the
> index?

The amount of time that the getIndex() would take compared to the rest of the DROP processing is miniscule. Yes, it creates a little garbage but that's about it.

Why? As I said earlier, less code is generally better. Less to understand, maintain, document, and test. In a scenario where we can have a single code path vs two, and the cost is a little garbage in uncommon scenarios, I'll always choose the former.

> I did. All the settings are gone, only TestConfigService still
> sets the interval now.

OptimizerTestBase.java:136
OptimizerTestBase.java:137
OptimizerTestBase.java:138

TestConfigService also declares TEXT_INDEX_PATH_KEY, which duplicates FTISI#INDEX_PATH_PROPERTY.

> What about locking that you were getting at?

As I mentioned previously, removing the key in FTSISI#deleteFromTree() is not enough. What happens if population is occurring the same time as deletion? Slightly before? Slightly after?

review: Needs Fixing

« Back to merge proposal