Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,9 @@
/**
* Common helpers for testing HBase that do not depend on specific server/etc. things.
*/
@InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.PHOENIX)
@InterfaceAudience.LimitedPrivate({HBaseInterfaceAudience.COPROC,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, in general let's not do this.

Mark it as IA.LP("Phoenix") is already a compromise, let's not add more compromise here...

Copy link
Contributor Author

@gjacoby126 gjacoby126 Jul 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand what the compromise is here?

For example, I wear three different hats. I write code for HBase itself, for Phoenix, and for my dayjob. You seem to be arguing that the type of test infrastructure I require depends on what hat I'm wearing, not on the code I'm writing. Are you saying that server-side component tests are good when I wear the HBase hat, a tolerated compromise when I wear the Phoenix hat, and bad when I wear the dayjob hat? And that that would be the case _even for the same code _ depending on where I intended to push it when it's done?

In my view, HBase tests that just exercise the client API should use the TestingHBaseCluster. Same with Phoenix tests, or dayjob tests that only rely on the HBase client API. Tests that exercise server-side components that need the extra control over server-side state should be able to do so. That's true whether the components are housed in an HBase repo, or a Phoenix repo, or any other project, open source or not.

HBase gives a lot of ways to create server side code, and they all need to be tested. The IA should recognize that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a native English speaker so sorry I can not understand what you mean.

What I say compromise here, is in HBASE-13126, we want to mark HBTU as IA.Private, but consider the current situation, at least we need to mark it as IA.LP("Phoenix"), as we are sure that Phoenix needs to test something internal to HBase. This is a compromise.

But if you offer your help on improving the new testing clsuter interface, I think maybe we could finish the support for Phoenix before 3.0.0 is out, then probably we could mark HBTU as IA.Private in 3.0.0 finally.

Thanks.

HBaseInterfaceAudience.REPLICATION,
HBaseInterfaceAudience.PHOENIX})
@InterfaceStability.Evolving
public class HBaseCommonTestingUtil {
protected static final Logger LOG = LoggerFactory.getLogger(HBaseCommonTestingUtil.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,9 @@
* <p>To preserve test data directories, pass the system property "hbase.testing.preserve.testdir"
* setting it to true.
*/
@InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.PHOENIX)
@InterfaceAudience.LimitedPrivate({HBaseInterfaceAudience.COPROC,
HBaseInterfaceAudience.REPLICATION,
HBaseInterfaceAudience.PHOENIX})
@InterfaceStability.Evolving
public class HBaseTestingUtil extends HBaseZKTestingUtil {

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/**
* 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
*
* http://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.hadoop.hbase.testing;

import org.apache.hadoop.hbase.HBaseInterfaceAudience;
import org.apache.hadoop.hbase.HBaseTestingUtil;
import org.apache.yetus.audience.InterfaceAudience;

/**
* This interface is intended for uncommon testing use cases which need to access internal state
* on an HBase minicluster. This may be necessary when testing code which runs within an HBase
* server process, such as coprocessors or replication endpoints. It provides access to the
* underlying {@link HBaseTestingUtil}, which its parent {@link TestingHBaseCluster} interface
* intentionally does not. External use cases that just need to test their code using the HBase
* client API should use {@link TestingHBaseCluster} instead.
*/
@InterfaceAudience.LimitedPrivate(
{HBaseInterfaceAudience.COPROC, HBaseInterfaceAudience.REPLICATION,
HBaseInterfaceAudience.PHOENIX})
public interface ServerTestingHBaseCluster extends TestingHBaseCluster {
/**
*
* @return the utility running the minicluster
*/
HBaseTestingUtil getUtil();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a no no.

Let's find another way to only expose the necessary methods for testing coprocessors and other server side hooks such as tags. The point here is, HBaseTestingUtil is a class, not an interface, which makes very hard for the HBase developper to make it stable while keeping it clean.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Apache9 - thanks, that's a good point. In the next revision of this patch, I will create an interface for HBTU (and probably its superclasses) so this interface doesn't return a concrete class. I'm going to be on vacation for about a week but will take this up when I get back.


/**
* Create a {@link ServerTestingHBaseCluster}. You need to call {@link #start()} of the returned
* {@link ServerTestingHBaseCluster} to actually start the mini testing cluster.
*/
static ServerTestingHBaseCluster create(TestingHBaseClusterOption option) {
return new TestingHBaseClusterImpl(option);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
import org.apache.hbase.thirdparty.com.google.common.util.concurrent.ThreadFactoryBuilder;

@InterfaceAudience.Private
class TestingHBaseClusterImpl implements TestingHBaseCluster {
class TestingHBaseClusterImpl implements ServerTestingHBaseCluster {

private final HBaseTestingUtil util = new HBaseTestingUtil();

Expand All @@ -55,6 +55,10 @@ public Configuration getConf() {
return util.getConfiguration();
}

public HBaseTestingUtil getUtil() {
return util;
}

private int getRegionServerIndex(ServerName serverName) {
// we have a small number of region servers, this should be fine for now.
List<RegionServerThread> servers = util.getMiniHBaseCluster().getRegionServerThreads();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@
* Helpers for testing HBase that do not depend on specific server/etc. things. The main difference
* from {@link HBaseCommonTestingUtil} is that we can start a zookeeper cluster.
*/
@InterfaceAudience.LimitedPrivate(HBaseInterfaceAudience.PHOENIX)
@InterfaceAudience.LimitedPrivate({HBaseInterfaceAudience.COPROC,
HBaseInterfaceAudience.REPLICATION,
HBaseInterfaceAudience.PHOENIX})
@InterfaceStability.Evolving
public class HBaseZKTestingUtil extends HBaseCommonTestingUtil {
private MiniZooKeeperCluster zkCluster;
Expand Down