Log more information about why compaction can not be planned

[keith-turner]

For the case when a tablet has more than tablet.file.max files and a compaction can not be planned, log more information about what went into the planning process.

[keith-turner]

Manually tested this by running the unit test for the class and examining the log output. Saw the following. Also had to adjust the log config for the test run.

[main] WARN  org.apache.accumulo.core.spi.compaction.DefaultCompactionPlanner [] - Unable to plan compaction for 42<< that has too many files. table.file.max:7 num_files:8 excluded_large_files:8 max_compaction_size:512.00M ratio_search_range:1.0625,3.0 
[main] DEBUG org.apache.accumulo.core.spi.compaction.DefaultCompactionPlanner [] - Failed planning details for 42<< examined_file_sizes:[] excluded_file_sizes:[29.80G, 59.60G, 3.73G, 953.67M, 953.67M, 14.90G, 7.45G, 1.86G]
[main] WARN  org.apache.accumulo.core.spi.compaction.DefaultCompactionPlanner [] - Unable to plan compaction for 42<< that has too many files. table.file.max:7 num_files:9 excluded_large_files:3 max_compaction_size:512.00M ratio_search_range:1.0625,3.0 
[main] DEBUG org.apache.accumulo.core.spi.compaction.DefaultCompactionPlanner [] - Failed planning details for 42<< examined_file_sizes:[10, 200, 4.10K, 86.13K, 1.77M, 37.09M] excluded_file_sizes:[778.98M, 335.48G, 15.98G]

[dlmarion]

Is there an issue with adding this method in a bugfix release? This would cause a build and runtime issue for any user that has their own CompactionPlanner implementation written against 2.1.4 and deploys it to a 2.1.3 instance, right? I'm wondering if this should be a default method that returns null (not throws a UnsupportedOperationException).

[keith-turner]

I'm wondering if this should be a default method that returns null (not throws a UnsupportedOperationException).

Returning null would not be the best long term behavior (like the behavior we would want in 4.0). I was worried about adding this, but chose this approach because a few lines up there is the following and I just followed its precedent.

    /**
     * @return The id of the namespace that the table is assigned to
     * @throws TableNotFoundException thrown when the namespace for a table cannot be calculated
     * @since 2.1.4
     */
    NamespaceId getNamespaceId() throws TableNotFoundException;

Whatever we do here, we probably need to do that consistently for all changes like this in 2.1.4.

[dlmarion]

I think for a 3.1 and a 4.0 we could remove the default method that returns null and just leave the method declaration in the interface. I think this change (and the other that you pointed out) potentially make a 2.1.4 api implementation not backwards compatible.

[keith-turner]

I think leaving these methods unimplemented w/o a default impl is best because that is the most straightforward way for a developer to know they need to do something if they implemented this interface. If a user were to implement this interface it would most likely be for testing and it would be easy for a developer to deai with test code not implementing a method. If we want to add default method I think it would be best to throw unsupported op exception instead of returning null. Returning null has the potential to cause runtime exceptions that are far from the method that returned null, making it harder to track down. The unsupported op exception makes the issue easy to track down. We have returned unsupported op exception in other places in the past and that turned out to be bad, but that was a bit different. In this case its a completely new method that no 2.1.3 code could have called, we are not all of a sudden throwing that exception for an existing method.

[ddanielr]

I agree that leaving the methods unimplemented is better since a user would run into a compile time error vs a runtime error in a production deployment.