ankitsinghal commented on a change in pull request #4069:
URL: https://github.com/apache/hbase/pull/4069#discussion_r795099022
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
##########
@@ -144,21 +145,42 @@ protected void loadSystemCoprocessors(Configuration conf,
String confKey) {
int currentSystemPriority = Coprocessor.PRIORITY_SYSTEM;
for (String className : defaultCPClasses) {
- String[] classNameAndPriority = className.split("\\|");
+ // After HBASE-23710 and HBASE-26714 when configuring for system
coprocessor, we accept
+ // an optional format of className|priority|path
+ String[] classNameToken = className.split("\\|");
boolean hasPriorityOverride = false;
- className = classNameAndPriority[0];
+ boolean hasPath = false;
+ className = classNameToken[0];
int overridePriority = Coprocessor.PRIORITY_SYSTEM;
- if (classNameAndPriority.length > 1){
- overridePriority = Integer.parseInt(classNameAndPriority[1]);
+ Path path = null;
+ if (classNameToken.length > 1 &&
!Strings.isNullOrEmpty(classNameToken[1])) {
+ overridePriority = Integer.parseInt(classNameToken[1]);
hasPriorityOverride = true;
}
+ if (classNameToken.length > 2 &&
!Strings.isNullOrEmpty(classNameToken[2])) {
+ path = new Path(classNameToken[2].trim());
+ hasPath = true;
+ }
className = className.trim();
if (findCoprocessor(className) != null) {
// If already loaded will just continue
LOG.warn("Attempted duplicate loading of " + className + "; skipped");
continue;
}
ClassLoader cl = this.getClass().getClassLoader();
+ // override the class loader if a path for the system coprocessor is
provided.
+ if (hasPath) {
+ String pathPrefix = UUID.randomUUID().toString();
Review comment:
Nit: instead use CoprocessHost.pathPrefix?
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorHost.java
##########
@@ -97,6 +102,108 @@ public void testDoubleLoadingAndPriorityValue() {
assertEquals(overridePriority, simpleEnv_v3.getPriority());
}
+ @Test
+ public void testLoadSystemCoprocessorWithPath() throws Exception {
+ Configuration conf = TEST_UTIL.getConfiguration();
+ final String key = "KEY";
+ final String testClassName = "TestSystemCoprocessor";
+ final String testClassNameWithPriorityAndPath = testClassName +
"PriorityAndPath";
+
+ File jarFile = buildCoprocessorJar(testClassName);
+ File jarFileWithPriorityAndPath =
buildCoprocessorJar(testClassNameWithPriorityAndPath);
+
+ try {
+ CoprocessorHost<RegionCoprocessor,
CoprocessorEnvironment<RegionCoprocessor>> host;
+ host = new CoprocessorHostForTest<>(conf);
+
+ // make a string of coprocessor with only priority
+ int overridePriority = Integer.MAX_VALUE - 1;
+ final String coprocessorWithPriority =
+ SimpleRegionObserverV3.class.getName() + "|" + overridePriority;
+ // make a string of coprocessor with path but no priority
+ final String coprocessorWithPath =
+ String.format("%s|%s|%s", testClassName, "",
jarFile.getAbsolutePath());
+ // make a string of coprocessor with priority and path
+ final String coprocessorWithPriorityAndPath = String
+ .format("%s|%s|%s", testClassNameWithPriorityAndPath,
(overridePriority - 1),
+ jarFileWithPriorityAndPath.getAbsolutePath());
+
+ // Try and load a system coprocessors
+ conf.setStrings(key, SimpleRegionObserverV2.class.getName(),
coprocessorWithPriority,
+ coprocessorWithPath, coprocessorWithPriorityAndPath);
+ host.loadSystemCoprocessors(conf, key);
+
+ // first loaded system coprocessor with default priority
+ CoprocessorEnvironment<?> simpleEnv =
+
host.findCoprocessorEnvironment(SimpleRegionObserverV2.class.getName());
+ assertNotNull(simpleEnv);
+ assertEquals(Coprocessor.PRIORITY_SYSTEM, simpleEnv.getPriority());
+
+ // external system coprocessor with default priority
+ CoprocessorEnvironment<?> coprocessorEnvironmentWithPath =
+ host.findCoprocessorEnvironment(testClassName);
+ assertNotNull(coprocessorEnvironmentWithPath);
+ assertEquals(Coprocessor.PRIORITY_SYSTEM + 1,
coprocessorEnvironmentWithPath.getPriority());
+
+ // system coprocessor with configured priority
+ CoprocessorEnvironment<?> coprocessorEnvironmentWithPriority =
+
host.findCoprocessorEnvironment(SimpleRegionObserverV3.class.getName());
+ assertNotNull(coprocessorEnvironmentWithPriority);
+ assertEquals(overridePriority,
coprocessorEnvironmentWithPriority.getPriority());
+
+ // external system coprocessor with override priority
+ CoprocessorEnvironment<?> coprocessorEnvironmentWithPriorityAndPath =
+ host.findCoprocessorEnvironment(testClassNameWithPriorityAndPath);
+ assertNotNull(coprocessorEnvironmentWithPriorityAndPath);
+ assertEquals(overridePriority - 1,
coprocessorEnvironmentWithPriorityAndPath.getPriority());
+ } finally {
+ if (jarFile.exists()) {
+ jarFile.delete();
+ }
+ if (jarFileWithPriorityAndPath.exists()) {
+ jarFileWithPriorityAndPath.delete();
+ }
+ }
+ }
+
+ @Test(expected = AssertionError.class)
+ public void testLoadSystemCoprocessorWithPathDoesNotExist() throws Exception
{
+ Configuration conf = TEST_UTIL.getConfiguration();
+ final String key = "KEY";
+ final String testClassName = "TestSystemCoprocessor";
+
+ CoprocessorHost<RegionCoprocessor,
CoprocessorEnvironment<RegionCoprocessor>> host;
+ host = new CoprocessorHostForTest<>(conf);
+
+ // make a string of coprocessor with path but no priority
+ final String coprocessorWithPath = testClassName + "||" + testClassName +
".jar";
+
+ // Try and load a system coprocessors
+ conf.setStrings(key, coprocessorWithPath);
+ // when loading non-exist with CoprocessorHostForTest host, it aborts with
AssertionError
+ host.loadSystemCoprocessors(conf, key);
+ }
+
+ @Test(expected = AssertionError.class)
+ public void testLoadSystemCoprocessorWithPriorityAndPath() throws Exception {
+ Configuration conf = TEST_UTIL.getConfiguration();
+ final String key = "KEY";
+ final String testClassName = "TestSystemCoprocessor";
+
+ CoprocessorHost<RegionCoprocessor,
CoprocessorEnvironment<RegionCoprocessor>> host;
+ host = new CoprocessorHostForTest<>(conf);
+
+ int overridePriority = Integer.MAX_VALUE - 1;
+ // make a string of coprocessor with path but no priority
Review comment:
Nit:
```suggestion
// make a string of coprocessor with path and priority
```
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
##########
@@ -144,21 +145,42 @@ protected void loadSystemCoprocessors(Configuration conf,
String confKey) {
int currentSystemPriority = Coprocessor.PRIORITY_SYSTEM;
for (String className : defaultCPClasses) {
- String[] classNameAndPriority = className.split("\\|");
+ // After HBASE-23710 and HBASE-26714 when configuring for system
coprocessor, we accept
+ // an optional format of className|priority|path
+ String[] classNameToken = className.split("\\|");
boolean hasPriorityOverride = false;
- className = classNameAndPriority[0];
+ boolean hasPath = false;
+ className = classNameToken[0];
int overridePriority = Coprocessor.PRIORITY_SYSTEM;
- if (classNameAndPriority.length > 1){
- overridePriority = Integer.parseInt(classNameAndPriority[1]);
+ Path path = null;
+ if (classNameToken.length > 1 &&
!Strings.isNullOrEmpty(classNameToken[1])) {
+ overridePriority = Integer.parseInt(classNameToken[1]);
hasPriorityOverride = true;
}
+ if (classNameToken.length > 2 &&
!Strings.isNullOrEmpty(classNameToken[2])) {
+ path = new Path(classNameToken[2].trim());
+ hasPath = true;
+ }
className = className.trim();
if (findCoprocessor(className) != null) {
// If already loaded will just continue
LOG.warn("Attempted duplicate loading of " + className + "; skipped");
continue;
}
ClassLoader cl = this.getClass().getClassLoader();
+ // override the class loader if a path for the system coprocessor is
provided.
+ if (hasPath) {
+ String pathPrefix = UUID.randomUUID().toString();
+ try {
+ cl = CoprocessorClassLoader.getClassLoader(path,
this.getClass().getClassLoader(),
+ pathPrefix, conf);
+ } catch (IOException ioe) {
+ // if system coprocessors cannot be obtained, we also about the
region server
+ LOG.error("Cannot fetch external System coprocessor class {} with
path {}", className,
+ path);
+ abortServer(className, ioe);
Review comment:
Can we move this logic under the below try{} catch{}, so that we don't
have multiple condition paths for "abortServer(className, ioe);"
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorHost.java
##########
@@ -97,6 +102,108 @@ public void testDoubleLoadingAndPriorityValue() {
assertEquals(overridePriority, simpleEnv_v3.getPriority());
}
+ @Test
+ public void testLoadSystemCoprocessorWithPath() throws Exception {
+ Configuration conf = TEST_UTIL.getConfiguration();
+ final String key = "KEY";
+ final String testClassName = "TestSystemCoprocessor";
+ final String testClassNameWithPriorityAndPath = testClassName +
"PriorityAndPath";
+
+ File jarFile = buildCoprocessorJar(testClassName);
+ File jarFileWithPriorityAndPath =
buildCoprocessorJar(testClassNameWithPriorityAndPath);
+
+ try {
+ CoprocessorHost<RegionCoprocessor,
CoprocessorEnvironment<RegionCoprocessor>> host;
+ host = new CoprocessorHostForTest<>(conf);
+
+ // make a string of coprocessor with only priority
+ int overridePriority = Integer.MAX_VALUE - 1;
+ final String coprocessorWithPriority =
+ SimpleRegionObserverV3.class.getName() + "|" + overridePriority;
+ // make a string of coprocessor with path but no priority
+ final String coprocessorWithPath =
+ String.format("%s|%s|%s", testClassName, "",
jarFile.getAbsolutePath());
+ // make a string of coprocessor with priority and path
+ final String coprocessorWithPriorityAndPath = String
+ .format("%s|%s|%s", testClassNameWithPriorityAndPath,
(overridePriority - 1),
+ jarFileWithPriorityAndPath.getAbsolutePath());
+
+ // Try and load a system coprocessors
+ conf.setStrings(key, SimpleRegionObserverV2.class.getName(),
coprocessorWithPriority,
+ coprocessorWithPath, coprocessorWithPriorityAndPath);
+ host.loadSystemCoprocessors(conf, key);
+
+ // first loaded system coprocessor with default priority
+ CoprocessorEnvironment<?> simpleEnv =
+
host.findCoprocessorEnvironment(SimpleRegionObserverV2.class.getName());
+ assertNotNull(simpleEnv);
+ assertEquals(Coprocessor.PRIORITY_SYSTEM, simpleEnv.getPriority());
+
+ // external system coprocessor with default priority
+ CoprocessorEnvironment<?> coprocessorEnvironmentWithPath =
+ host.findCoprocessorEnvironment(testClassName);
+ assertNotNull(coprocessorEnvironmentWithPath);
+ assertEquals(Coprocessor.PRIORITY_SYSTEM + 1,
coprocessorEnvironmentWithPath.getPriority());
+
+ // system coprocessor with configured priority
+ CoprocessorEnvironment<?> coprocessorEnvironmentWithPriority =
+
host.findCoprocessorEnvironment(SimpleRegionObserverV3.class.getName());
+ assertNotNull(coprocessorEnvironmentWithPriority);
+ assertEquals(overridePriority,
coprocessorEnvironmentWithPriority.getPriority());
+
+ // external system coprocessor with override priority
+ CoprocessorEnvironment<?> coprocessorEnvironmentWithPriorityAndPath =
+ host.findCoprocessorEnvironment(testClassNameWithPriorityAndPath);
+ assertNotNull(coprocessorEnvironmentWithPriorityAndPath);
+ assertEquals(overridePriority - 1,
coprocessorEnvironmentWithPriorityAndPath.getPriority());
+ } finally {
+ if (jarFile.exists()) {
+ jarFile.delete();
+ }
+ if (jarFileWithPriorityAndPath.exists()) {
+ jarFileWithPriorityAndPath.delete();
+ }
+ }
+ }
+
+ @Test(expected = AssertionError.class)
+ public void testLoadSystemCoprocessorWithPathDoesNotExist() throws Exception
{
+ Configuration conf = TEST_UTIL.getConfiguration();
+ final String key = "KEY";
+ final String testClassName = "TestSystemCoprocessor";
+
+ CoprocessorHost<RegionCoprocessor,
CoprocessorEnvironment<RegionCoprocessor>> host;
+ host = new CoprocessorHostForTest<>(conf);
+
+ // make a string of coprocessor with path but no priority
+ final String coprocessorWithPath = testClassName + "||" + testClassName +
".jar";
+
+ // Try and load a system coprocessors
+ conf.setStrings(key, coprocessorWithPath);
+ // when loading non-exist with CoprocessorHostForTest host, it aborts with
AssertionError
+ host.loadSystemCoprocessors(conf, key);
+ }
+
+ @Test(expected = AssertionError.class)
+ public void testLoadSystemCoprocessorWithPriorityAndPath() throws Exception {
Review comment:
Nit
```suggestion
public void testLoadSystemCoprocessorWithPathDoesNotExistAndPriority()
throws Exception {
```
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java
##########
@@ -144,21 +145,42 @@ protected void loadSystemCoprocessors(Configuration conf,
String confKey) {
int currentSystemPriority = Coprocessor.PRIORITY_SYSTEM;
for (String className : defaultCPClasses) {
- String[] classNameAndPriority = className.split("\\|");
+ // After HBASE-23710 and HBASE-26714 when configuring for system
coprocessor, we accept
+ // an optional format of className|priority|path
+ String[] classNameToken = className.split("\\|");
boolean hasPriorityOverride = false;
- className = classNameAndPriority[0];
+ boolean hasPath = false;
+ className = classNameToken[0];
int overridePriority = Coprocessor.PRIORITY_SYSTEM;
- if (classNameAndPriority.length > 1){
- overridePriority = Integer.parseInt(classNameAndPriority[1]);
+ Path path = null;
+ if (classNameToken.length > 1 &&
!Strings.isNullOrEmpty(classNameToken[1])) {
+ overridePriority = Integer.parseInt(classNameToken[1]);
hasPriorityOverride = true;
}
+ if (classNameToken.length > 2 &&
!Strings.isNullOrEmpty(classNameToken[2])) {
+ path = new Path(classNameToken[2].trim());
+ hasPath = true;
+ }
className = className.trim();
if (findCoprocessor(className) != null) {
// If already loaded will just continue
LOG.warn("Attempted duplicate loading of " + className + "; skipped");
continue;
}
ClassLoader cl = this.getClass().getClassLoader();
+ // override the class loader if a path for the system coprocessor is
provided.
+ if (hasPath) {
+ String pathPrefix = UUID.randomUUID().toString();
+ try {
+ cl = CoprocessorClassLoader.getClassLoader(path,
this.getClass().getClassLoader(),
+ pathPrefix, conf);
+ } catch (IOException ioe) {
+ // if system coprocessors cannot be obtained, we also about the
region server
+ LOG.error("Cannot fetch external System coprocessor class {} with
path {}", className,
+ path);
+ abortServer(className, ioe);
Review comment:
Can we move this logic under the original try{} catch{}, so that we
don't have multiple condition for "abortServer(className, ioe);"
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
