cshannon commented on code in PR #4659: URL: https://github.com/apache/accumulo/pull/4659#discussion_r1639963033
########## test/src/main/java/org/apache/accumulo/test/ample/FlakyInterceptor.java: ########## @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.accumulo.test.ample; + +import static org.apache.accumulo.core.client.ConditionalWriter.Status.UNKNOWN; +import static org.apache.accumulo.core.util.LazySingletons.RANDOM; + +import java.util.ArrayList; +import java.util.Iterator; +import java.util.List; + +import org.apache.accumulo.core.client.ConditionalWriter; +import org.apache.accumulo.core.data.ConditionalMutation; +import org.apache.accumulo.test.ample.metadata.ConditionalWriterInterceptor; +import org.apache.hadoop.io.Text; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class FlakyInterceptor implements ConditionalWriterInterceptor { Review Comment: This probably needs more discussion but I think the best approach might be to set up the surefire/failsure plugins differently. We really shouldn't need to run most tests with the `rerunFailingTestsCount` flag set as that just hides issues. I think a better approach would be to set up multiple profiles or executions for the test plugins and by default include all tests and don't set the `rerunFailingTestsCount` flag. Then if we come across flaky tests, we could either try and fix them or if we can't we can `emporarily or permanently add them to another profile where we can set that flag (maybe we need our own -D property for the config). In general, the setting is certainly useful because sometimes tests are just hard to write and are flaky so it can be nice to not fail, but it seems like it should be a temporary solution until the test is fixed and not permanent. Keeping it on it definitely can be a problem as it will also hide true errors that need to be fixed but are intermittent. @ctubbsii - Any thoughts on this and the best way we could set up our Maven test plugins with two different modes/profiles where we only execute a small list of tests with `rerunFailingTestsCount` flag if it's determined we need to? Or do you have another idea? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@accumulo.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
