You are viewing a plain text version of this content. The canonical link for it is here.
Posted to codereview@trafodion.apache.org by traflm <gi...@git.apache.org> on 2018/04/21 15:30:49 UTC
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
GitHub user traflm opened a pull request:
https://github.com/apache/trafodion/pull/1532
[TRAFODION-3026] add create option storage policy
Some user want Trafodion to support HSM provided by Hadoop 2.6.0. Tables can be stored on different type of storage hardware (SSD/HDD). Hadoop provides HSM in HDFS, but Hbase doesn't provide the support until HBase 2.0. There is HBASE-19858 and plan to support HSM in HBase 1.5.0. But still not released.
So before that HBase feature availible, we now use HDFS API to directly set the storage policy on Hbase table path.
Trafodion adds new HBASE_OPTIONS to support this feature.
So when creating a Trafodion table, it is possible to specify the storage policy (by default, HDFS provide hot/warm/cold/one_ssd/all_ssd etc)
This provide a basic support of user's requirement.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/traflm/trafodion TRAFODION-3026
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafodion/pull/1532.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1532
----
commit 5724227c48f1fa33e993a5642803792fb0478867
Author: Liu Ming <ov...@...>
Date: 2018-04-21T15:26:52Z
[TRAFODION-3025] add create option storage policy
commit ad05d5c0c6be22760266a0a2b365d4a7c5d2747a
Author: Liu Ming <ov...@...>
Date: 2018-04-21T15:27:44Z
remove typos
----
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1532#discussion_r193921336
--- Diff: core/sqf/src/seatrans/hbase-trx/src/main/java/org/apache/hadoop/hbase/client/transactional/TransactionManager.java ---
@@ -2996,6 +3022,11 @@ else if (tableOption.equalsIgnoreCase("USE_DEFAULT"))
(Long.parseLong(tableOption));
returnStatus.setTableDescriptorChanged();
break ;
+ case HBASE_HDFS_STORAGE_POLICY:
--- End diff --
Hi, Prashanth, the hdfs path of the table is available only after the table is created. And it is easier to set this in alter, since it directly support the SQL ALTER TABLE to change this option. But I was planning to test and support other DDL including ALTER in phase two. But add code here has one consideration of the reason above.
Let me consider how to test this out, as I understand, it will never be late, since the policy can be changed at anytime.
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by prashanth-vasudev <gi...@git.apache.org>.
Github user prashanth-vasudev commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1532#discussion_r193887315
--- Diff: core/sql/src/main/java/org/trafodion/sql/HBaseClient.java ---
@@ -538,7 +564,21 @@ public boolean createk(String tblName, Object[] tableOptions,
admin.createTable(desc);
}
}
- admin.close();
+
+ if(setDescRet!= null)
+ {
+ if(setDescRet.storagePolicyChanged())
+ {
+ Object tableOptionsStoragePolicy[] = new Object[HBASE_HDFS_STORAGE_POLICY+1];
+ for(int i=0; i<HBASE_HDFS_STORAGE_POLICY; i++)
+ tableOptionsStoragePolicy[i]="";
+ tableOptionsStoragePolicy[HBASE_HDFS_STORAGE_POLICY]=(String)setDescRet.storagePolicy_ ;
+ tableOptionsStoragePolicy[HBASE_NAME]=(String)tblName;
+ alter(tblName,tableOptionsStoragePolicy,transID);
--- End diff --
Transactional Alter is not supported yet. This is due to additional changes required to support transactional rollback of a table. Can you make coprocessor call outside of transactional alter?
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1532#discussion_r184734061
--- Diff: core/sql/src/main/java/org/trafodion/sql/HBaseClient.java ---
@@ -552,10 +578,84 @@ public boolean createk(String tblName, Object[] tableOptions,
admin.createTable(desc);
}
}
- admin.close();
+
+ if(setDescRet!= null)
+ if(setDescRet.storagePolicyChanged())
+ {
+ //change the HDFS storage policy
+ //get the HBase table path
+ String hbaseRoot = config.get("hbase.rootdir");
+ FileSystem fs = FileSystem.get(config);
+ //Construct the HDFS dir
+ //find out if namespace is there
+ String[] parts = tblName.split(":");
+ String namespacestr="";
+
+ //guess the path pattern
+ //different HBase version may have different path pattern
+ //There is no interface to get this information using HBase User API
+ //Since it is HBase internal behavior
+ //At present, before HBase 2.0 release and before HBASE-19858 released in HBase 1.5.0
+ //Trafodion here need a trick to guess
+ String fullPath = hbaseRoot + "/data/" ;
+ String fullPath2 = hbaseRoot + "/data/default/";
+
+ //check if fullPath2 exist
+ if(fs.exists(new Path(fullPath2)))
+ fullPath = fullPath2;
+
+ if(parts.length >1) //have namespace
+ fullPath = fullPath + parts[0] + "/" + parts[1];
+ else
+ fullPath = fullPath + tblName;
+
+ if (logger.isDebugEnabled()) logger.debug("createk table fullPath is " + fullPath);
+
+ String invokeret = invokeSetStoragePolicy(fs, fullPath, setDescRet.storagePolicy_ ) ;
+
+ if( invokeret != null)
+ {
+ //error handling
+ admin.close();
+ throw new IOException(invokeret);
+ }
+ }
+
+ admin.close();
return true;
}
+ private static String invokeSetStoragePolicy(final FileSystem fs, final String pathstr,
+ final String storagePolicy) {
+ String ret = null;
+ Path path = new Path(pathstr);
+ Method m = null;
+ try {
+ m = fs.getClass().getDeclaredMethod("setStoragePolicy",
+ new Class<?>[] { Path.class, String.class });
+ m.setAccessible(true);
+ } catch (NoSuchMethodException e) {
+ ret = "FileSystem doesn't support setStoragePolicy";
+ m = null;
+ } catch (SecurityException e) {
+ ret = "No access to setStoragePolicy on FileSystem from the SecurityManager";
+ m = null; // could happen on setAccessible() or getDeclaredMethod()
+ }
+ if (m != null) {
+ try {
+ m.invoke(fs, path, storagePolicy);
+ if (logger.isDebugEnabled()) {
+ logger.debug("Set storagePolicy=" + storagePolicy + " for path=" + path);
+ }
+ } catch (Exception e) {
+ logger.error("invoke set storage policy error : " + e);
--- End diff --
Thanks for making the changes as requested. You might have overlooked this. Changing '+' to ',' (make it as a parameter) would help to see the stack in the log file. Other wise only the exception name alone will be logged.
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by prashanth-vasudev <gi...@git.apache.org>.
Github user prashanth-vasudev commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1532#discussion_r193885236
--- Diff: core/sqf/src/seatrans/hbase-trx/src/main/java/org/apache/hadoop/hbase/client/transactional/TransactionManager.java ---
@@ -3226,5 +3260,65 @@ public RecoveryRequestResponse call(TrxRegionService instance) throws IOExceptio
return resultArray[0].getResultList();
}
+
+ public void setStoragePolicy(String tblName, String policy)
+ throws IOException {
+
+ int retryCount = 0;
+ int retrySleep = TM_SLEEP;
+ boolean retry = false;
+ try {
+ Table tbl = connection.getTable(TableName.valueOf(tblName));
+ String rowkey = "0";
+ CoprocessorRpcChannel channel = tbl.coprocessorService(rowkey.getBytes());
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrxRegionService.BlockingInterface service =
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrxRegionService.newBlockingStub(channel);
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyRequest.Builder request =
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyRequest.newBuilder();
+ String hbaseRoot = config.get("hbase.rootdir");
+ FileSystem fs = FileSystem.get(config);
+ //Construct the HDFS dir
+ //find out if namespace is there
+ String[] parts = tblName.split(":");
+ String namespacestr="";
--- End diff --
One concern here is we make assumptions of hbase internal directory structure for the table. This assumption may not hold good between releases. Is there any public api that can be used or some uniform way?
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by prashanth-vasudev <gi...@git.apache.org>.
Github user prashanth-vasudev commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1532#discussion_r193888426
--- Diff: core/sqf/src/seatrans/hbase-trx/src/main/java/org/apache/hadoop/hbase/client/transactional/TransactionManager.java ---
@@ -2996,6 +3022,11 @@ else if (tableOption.equalsIgnoreCase("USE_DEFAULT"))
(Long.parseLong(tableOption));
returnStatus.setTableDescriptorChanged();
break ;
+ case HBASE_HDFS_STORAGE_POLICY:
--- End diff --
General question. Is it too late to change (alter ) storage policy of a table after it is created. Should this is part of create?
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by prashanth-vasudev <gi...@git.apache.org>.
Github user prashanth-vasudev commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1532#discussion_r193960465
--- Diff: core/sql/src/main/java/org/trafodion/sql/HBaseClient.java ---
@@ -538,7 +564,21 @@ public boolean createk(String tblName, Object[] tableOptions,
admin.createTable(desc);
}
}
- admin.close();
+
+ if(setDescRet!= null)
+ {
+ if(setDescRet.storagePolicyChanged())
+ {
+ Object tableOptionsStoragePolicy[] = new Object[HBASE_HDFS_STORAGE_POLICY+1];
+ for(int i=0; i<HBASE_HDFS_STORAGE_POLICY; i++)
+ tableOptionsStoragePolicy[i]="";
+ tableOptionsStoragePolicy[HBASE_HDFS_STORAGE_POLICY]=(String)setDescRet.storagePolicy_ ;
+ tableOptionsStoragePolicy[HBASE_NAME]=(String)tblName;
+ alter(tblName,tableOptionsStoragePolicy,transID);
--- End diff --
Thanks for the responses. Please verify if transactional alter side of the code is functioning. We can enhance the alter in steps. Thank you.
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1532#discussion_r184141237
--- Diff: core/sql/src/main/java/org/trafodion/sql/HBaseClient.java ---
@@ -552,10 +578,84 @@ public boolean createk(String tblName, Object[] tableOptions,
admin.createTable(desc);
}
}
- admin.close();
+
+ if(setDescRet!= null)
+ if(setDescRet.storagePolicyChanged())
+ {
+ //change the HDFS storage policy
+ //get the HBase table path
+ String hbaseRoot = config.get("hbase.rootdir");
+ FileSystem fs = FileSystem.get(config);
+ //Construct the HDFS dir
+ //find out if namespace is there
+ String[] parts = tblName.split(":");
+ String namespacestr="";
+
+ //guess the path pattern
+ //different HBase version may have different path pattern
+ //There is no interface to get this information using HBase User API
+ //Since it is HBase internal behavior
+ //At present, before HBase 2.0 release and before HBASE-19858 released in HBase 1.5.0
+ //Trafodion here need a trick to guess
+ String fullPath = hbaseRoot + "/data/" ;
+ String fullPath2 = hbaseRoot + "/data/default/";
+
+ //check if fullPath2 exist
+ if(fs.exists(new Path(fullPath2)))
+ fullPath = fullPath2;
+
+ if(parts.length >1) //have namespace
+ fullPath = fullPath + parts[0] + "/" + parts[1];
+ else
+ fullPath = fullPath + tblName;
+
+ if (logger.isDebugEnabled()) logger.debug("createk table fullPath is " + fullPath);
+
+ String invokeret = invokeSetStoragePolicy(fs, fullPath, setDescRet.storagePolicy_ ) ;
+
+ if( invokeret != null)
+ {
+ //error handling
+ admin.close();
+ throw new IOException(invokeret);
+ }
+ }
+
+ admin.close();
return true;
}
+ private static String invokeSetStoragePolicy(final FileSystem fs, final String pathstr,
+ final String storagePolicy) {
+ String ret = null;
+ Path path = new Path(pathstr);
+ Method m = null;
+ try {
+ m = fs.getClass().getDeclaredMethod("setStoragePolicy",
+ new Class<?>[] { Path.class, String.class });
+ m.setAccessible(true);
+ } catch (NoSuchMethodException e) {
+ ret = "FileSystem doesn't support setStoragePolicy";
+ m = null;
+ } catch (SecurityException e) {
+ ret = "No access to setStoragePolicy on FileSystem from the SecurityManager";
+ m = null; // could happen on setAccessible() or getDeclaredMethod()
+ }
+ if (m != null) {
+ try {
+ m.invoke(fs, path, storagePolicy);
+ if (logger.isDebugEnabled()) {
+ logger.debug("Set storagePolicy=" + storagePolicy + " for path=" + path);
+ }
+ } catch (Exception e) {
--- End diff --
Please try to avoid catching generic Exception if possible
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1532#discussion_r193922170
--- Diff: core/sql/src/main/java/org/trafodion/sql/HBaseClient.java ---
@@ -538,7 +564,21 @@ public boolean createk(String tblName, Object[] tableOptions,
admin.createTable(desc);
}
}
- admin.close();
+
+ if(setDescRet!= null)
+ {
+ if(setDescRet.storagePolicyChanged())
+ {
+ Object tableOptionsStoragePolicy[] = new Object[HBASE_HDFS_STORAGE_POLICY+1];
+ for(int i=0; i<HBASE_HDFS_STORAGE_POLICY; i++)
+ tableOptionsStoragePolicy[i]="";
+ tableOptionsStoragePolicy[HBASE_HDFS_STORAGE_POLICY]=(String)setDescRet.storagePolicy_ ;
+ tableOptionsStoragePolicy[HBASE_NAME]=(String)tblName;
+ alter(tblName,tableOptionsStoragePolicy,transID);
--- End diff --
I am not farmiliar with DDL transaction, need some time to understand this and make changes here.
But maybe we can make this work in different phases, in order to have it basically working.
I don't think this action need to do rollback, since if the CREATE fail, the path will be removed, so no need to set the storage policy back and then remove.
Removing the file should be part of the original CREATE TABLE process, but maybe I am misunderstanding here.
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1532#discussion_r193876755
--- Diff: core/sqf/src/seatrans/hbase-trx/src/main/java/org/apache/hadoop/hbase/client/transactional/TransactionManager.java ---
@@ -3226,5 +3260,65 @@ public RecoveryRequestResponse call(TrxRegionService instance) throws IOExceptio
return resultArray[0].getResultList();
}
+
+ public void setStoragePolicy(String tblName, String policy)
+ throws IOException {
+
+ int retryCount = 0;
+ int retrySleep = TM_SLEEP;
+ boolean retry = false;
+ try {
+ Table tbl = connection.getTable(TableName.valueOf(tblName));
+ String rowkey = "0";
+ CoprocessorRpcChannel channel = tbl.coprocessorService(rowkey.getBytes());
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrxRegionService.BlockingInterface service =
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrxRegionService.newBlockingStub(channel);
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyRequest.Builder request =
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyRequest.newBuilder();
+ String hbaseRoot = config.get("hbase.rootdir");
+ FileSystem fs = FileSystem.get(config);
+ //Construct the HDFS dir
+ //find out if namespace is there
+ String[] parts = tblName.split(":");
+ String namespacestr="";
+ String fullPath = hbaseRoot + "/data/" ;
+ String fullPath2 = hbaseRoot + "/data/default/";
+ if(fs.exists(new Path(fullPath2)))
+ fullPath = fullPath2;
+
+ if(parts.length >1) //have namespace
+ fullPath = fullPath + parts[0] + "/" + parts[1];
+ else
+ fullPath = fullPath + tblName;
+
+ request.setPath(fullPath);
+ request.setPolicy(policy);
+
+ do {
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyResponse ret =
+ service.setStoragePolicy(null,request.build());
+
+ //handle result and error
+ if( ret == null)
+ {
+ LOG.error("setStoragePolicy Response ret null ");
+ }
+ else if (ret.getStatus() == false)
+ {
+ LOG.error("setStoragePolicy Response ret false: " + ret.getException());
+ throw new IOException(ret.getException());
+ }
+ if(retryCount == RETRY_ATTEMPTS)
+ {
+ throw new IOException("coprocessor not response");
+ }
+ if (retry)
+ retrySleep = retry(retrySleep);
+ } while (retry && retryCount++ < RETRY_ATTEMPTS);
+ }
+ catch (Exception e) {
+ throw new IOException(e);
+ }
+ }
--- End diff --
Also, can you please consider coding the LOG.error as follows with 2 arguments
LOG.error(<message>, <Exception>)
Note the ',' instead of '+'
LOG.error(<nessage + e>), you would only the text of the error message and will not report the stack trace of the exception
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/trafodion/pull/1532
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1532#discussion_r193921620
--- Diff: core/sqf/src/seatrans/hbase-trx/src/main/java/org/apache/hadoop/hbase/client/transactional/TransactionManager.java ---
@@ -3226,5 +3260,65 @@ public RecoveryRequestResponse call(TrxRegionService instance) throws IOExceptio
return resultArray[0].getResultList();
}
+
+ public void setStoragePolicy(String tblName, String policy)
+ throws IOException {
+
+ int retryCount = 0;
+ int retrySleep = TM_SLEEP;
+ boolean retry = false;
+ try {
+ Table tbl = connection.getTable(TableName.valueOf(tblName));
+ String rowkey = "0";
+ CoprocessorRpcChannel channel = tbl.coprocessorService(rowkey.getBytes());
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrxRegionService.BlockingInterface service =
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrxRegionService.newBlockingStub(channel);
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyRequest.Builder request =
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyRequest.newBuilder();
+ String hbaseRoot = config.get("hbase.rootdir");
+ FileSystem fs = FileSystem.get(config);
+ //Construct the HDFS dir
+ //find out if namespace is there
+ String[] parts = tblName.split(":");
+ String namespacestr="";
--- End diff --
The API is only available at HBase 2.0. But I don't know when Trafodion will support HBase 2.0. And this feature is required by a customer, so we cannnot wait for months. This is a temp solution, once we move to HBase 2.0, most of these code can be eliminated and using HBase public API.
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1532#discussion_r184142826
--- Diff: core/sql/src/main/java/org/trafodion/sql/HBaseClient.java ---
@@ -552,10 +578,84 @@ public boolean createk(String tblName, Object[] tableOptions,
admin.createTable(desc);
}
}
- admin.close();
+
+ if(setDescRet!= null)
+ if(setDescRet.storagePolicyChanged())
+ {
+ //change the HDFS storage policy
+ //get the HBase table path
+ String hbaseRoot = config.get("hbase.rootdir");
+ FileSystem fs = FileSystem.get(config);
+ //Construct the HDFS dir
+ //find out if namespace is there
+ String[] parts = tblName.split(":");
+ String namespacestr="";
+
+ //guess the path pattern
+ //different HBase version may have different path pattern
+ //There is no interface to get this information using HBase User API
+ //Since it is HBase internal behavior
+ //At present, before HBase 2.0 release and before HBASE-19858 released in HBase 1.5.0
+ //Trafodion here need a trick to guess
+ String fullPath = hbaseRoot + "/data/" ;
+ String fullPath2 = hbaseRoot + "/data/default/";
+
+ //check if fullPath2 exist
+ if(fs.exists(new Path(fullPath2)))
+ fullPath = fullPath2;
+
+ if(parts.length >1) //have namespace
+ fullPath = fullPath + parts[0] + "/" + parts[1];
+ else
+ fullPath = fullPath + tblName;
+
+ if (logger.isDebugEnabled()) logger.debug("createk table fullPath is " + fullPath);
+
+ String invokeret = invokeSetStoragePolicy(fs, fullPath, setDescRet.storagePolicy_ ) ;
+
+ if( invokeret != null)
+ {
+ //error handling
+ admin.close();
+ throw new IOException(invokeret);
+ }
+ }
+
+ admin.close();
return true;
}
+ private static String invokeSetStoragePolicy(final FileSystem fs, final String pathstr,
+ final String storagePolicy) {
+ String ret = null;
+ Path path = new Path(pathstr);
+ Method m = null;
+ try {
+ m = fs.getClass().getDeclaredMethod("setStoragePolicy",
+ new Class<?>[] { Path.class, String.class });
+ m.setAccessible(true);
+ } catch (NoSuchMethodException e) {
+ ret = "FileSystem doesn't support setStoragePolicy";
+ m = null;
+ } catch (SecurityException e) {
+ ret = "No access to setStoragePolicy on FileSystem from the SecurityManager";
+ m = null; // could happen on setAccessible() or getDeclaredMethod()
+ }
+ if (m != null) {
+ try {
+ m.invoke(fs, path, storagePolicy);
+ if (logger.isDebugEnabled()) {
+ logger.debug("Set storagePolicy=" + storagePolicy + " for path=" + path);
+ }
+ } catch (Exception e) {
+ logger.error("invoke set storage policy error : " + e);
+ ret = "invoke set storage policy error : " + e.getMessage();
--- End diff --
To preserve the call stack, either let the method to throw all the exceptions. If the generic Exception needs to be caught due to underlying API, throw new IOException(e). Let the caller also throw this IOException to be handled in the JNI layer
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1532#discussion_r193880363
--- Diff: core/sqf/src/seatrans/hbase-trx/src/main/java/org/apache/hadoop/hbase/coprocessor/transactional/TrxRegionEndpoint.java.tmpl ---
@@ -2404,6 +2410,72 @@ CoprocessorService, Coprocessor {
done.run(TlogDel_response);
}
+ public void setStoragePolicy(RpcController controller,
+ TrafSetStoragePolicyRequest request,
+ RpcCallback<TrafSetStoragePolicyResponse> done) {
+ String path = request.getPath();
+ String policy = request.getPolicy();
+ if (LOG.isTraceEnabled()) LOG.trace("setStoragePolicy ENTRY. path " + path + " policy " + policy );
+
+ IOException t=null;
+ try {
+ invokeSetStoragePolicy(fs, path, policy);
+ }
+ catch (IOException e) {
+ t = e;
+ }
+
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyResponse.Builder setStoragePolicyResponseBuilder =
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyResponse.newBuilder();
+
+ if(t != null)
+ {
+ LOG.error("setStoragePolicy error : " + t.toString() );
+ setStoragePolicyResponseBuilder.setStatus(false);
+ setStoragePolicyResponseBuilder.setException(t.toString());
+ }
+ else
+ {
+ setStoragePolicyResponseBuilder.setStatus(true);
+ setStoragePolicyResponseBuilder.setException("");
+ }
+
+ TrafSetStoragePolicyResponse resp = setStoragePolicyResponseBuilder.build();
+
+ done.run(resp);
+
+ }
+
+ private static void invokeSetStoragePolicy(final FileSystem fs, final String pathstr,
+ final String storagePolicy)
+ throws IOException {
+ Path path = new Path(pathstr);
+ Method m = null;
+ try {
+ m = fs.getClass().getDeclaredMethod("setStoragePolicy",
+ new Class<?>[] { Path.class, String.class });
+ m.setAccessible(true);
--- End diff --
To avoid increased path length, consider issue reflection API to get the method Id once in static block
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1532#discussion_r193875770
--- Diff: core/sqf/src/seatrans/hbase-trx/src/main/java/org/apache/hadoop/hbase/client/transactional/TransactionManager.java ---
@@ -3226,5 +3260,65 @@ public RecoveryRequestResponse call(TrxRegionService instance) throws IOExceptio
return resultArray[0].getResultList();
}
+
+ public void setStoragePolicy(String tblName, String policy)
+ throws IOException {
+
+ int retryCount = 0;
+ int retrySleep = TM_SLEEP;
+ boolean retry = false;
+ try {
+ Table tbl = connection.getTable(TableName.valueOf(tblName));
+ String rowkey = "0";
+ CoprocessorRpcChannel channel = tbl.coprocessorService(rowkey.getBytes());
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrxRegionService.BlockingInterface service =
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrxRegionService.newBlockingStub(channel);
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyRequest.Builder request =
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyRequest.newBuilder();
+ String hbaseRoot = config.get("hbase.rootdir");
+ FileSystem fs = FileSystem.get(config);
+ //Construct the HDFS dir
+ //find out if namespace is there
+ String[] parts = tblName.split(":");
+ String namespacestr="";
+ String fullPath = hbaseRoot + "/data/" ;
+ String fullPath2 = hbaseRoot + "/data/default/";
+ if(fs.exists(new Path(fullPath2)))
+ fullPath = fullPath2;
+
+ if(parts.length >1) //have namespace
+ fullPath = fullPath + parts[0] + "/" + parts[1];
+ else
+ fullPath = fullPath + tblName;
+
+ request.setPath(fullPath);
+ request.setPolicy(policy);
+
+ do {
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyResponse ret =
+ service.setStoragePolicy(null,request.build());
+
+ //handle result and error
+ if( ret == null)
+ {
+ LOG.error("setStoragePolicy Response ret null ");
+ }
+ else if (ret.getStatus() == false)
+ {
+ LOG.error("setStoragePolicy Response ret false: " + ret.getException());
+ throw new IOException(ret.getException());
+ }
+ if(retryCount == RETRY_ATTEMPTS)
+ {
+ throw new IOException("coprocessor not response");
+ }
+ if (retry)
+ retrySleep = retry(retrySleep);
+ } while (retry && retryCount++ < RETRY_ATTEMPTS);
+ }
+ catch (Exception e) {
+ throw new IOException(e);
+ }
+ }
--- End diff --
retry is never set to true. So, it is not clear when the request would be retried
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1532#discussion_r193921823
--- Diff: core/sqf/src/seatrans/hbase-trx/src/main/java/org/apache/hadoop/hbase/coprocessor/transactional/TrxRegionEndpoint.java.tmpl ---
@@ -2404,6 +2410,72 @@ CoprocessorService, Coprocessor {
done.run(TlogDel_response);
}
+ public void setStoragePolicy(RpcController controller,
+ TrafSetStoragePolicyRequest request,
+ RpcCallback<TrafSetStoragePolicyResponse> done) {
+ String path = request.getPath();
+ String policy = request.getPolicy();
+ if (LOG.isTraceEnabled()) LOG.trace("setStoragePolicy ENTRY. path " + path + " policy " + policy );
+
+ IOException t=null;
+ try {
+ invokeSetStoragePolicy(fs, path, policy);
+ }
+ catch (IOException e) {
+ t = e;
+ }
+
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyResponse.Builder setStoragePolicyResponseBuilder =
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyResponse.newBuilder();
+
+ if(t != null)
+ {
+ LOG.error("setStoragePolicy error : " + t.toString() );
+ setStoragePolicyResponseBuilder.setStatus(false);
+ setStoragePolicyResponseBuilder.setException(t.toString());
+ }
+ else
+ {
+ setStoragePolicyResponseBuilder.setStatus(true);
+ setStoragePolicyResponseBuilder.setException("");
+ }
+
+ TrafSetStoragePolicyResponse resp = setStoragePolicyResponseBuilder.build();
+
+ done.run(resp);
+
+ }
+
+ private static void invokeSetStoragePolicy(final FileSystem fs, final String pathstr,
+ final String storagePolicy)
+ throws IOException {
+ Path path = new Path(pathstr);
+ Method m = null;
+ try {
+ m = fs.getClass().getDeclaredMethod("setStoragePolicy",
+ new Class<?>[] { Path.class, String.class });
+ m.setAccessible(true);
--- End diff --
Yes, Selva, this is a good suggestion.
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by selvaganesang <gi...@git.apache.org>.
Github user selvaganesang commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1532#discussion_r184141510
--- Diff: core/sql/src/main/java/org/trafodion/sql/HBaseClient.java ---
@@ -552,10 +578,84 @@ public boolean createk(String tblName, Object[] tableOptions,
admin.createTable(desc);
}
}
- admin.close();
+
+ if(setDescRet!= null)
+ if(setDescRet.storagePolicyChanged())
+ {
+ //change the HDFS storage policy
+ //get the HBase table path
+ String hbaseRoot = config.get("hbase.rootdir");
+ FileSystem fs = FileSystem.get(config);
+ //Construct the HDFS dir
+ //find out if namespace is there
+ String[] parts = tblName.split(":");
+ String namespacestr="";
+
+ //guess the path pattern
+ //different HBase version may have different path pattern
+ //There is no interface to get this information using HBase User API
+ //Since it is HBase internal behavior
+ //At present, before HBase 2.0 release and before HBASE-19858 released in HBase 1.5.0
+ //Trafodion here need a trick to guess
+ String fullPath = hbaseRoot + "/data/" ;
+ String fullPath2 = hbaseRoot + "/data/default/";
+
+ //check if fullPath2 exist
+ if(fs.exists(new Path(fullPath2)))
+ fullPath = fullPath2;
+
+ if(parts.length >1) //have namespace
+ fullPath = fullPath + parts[0] + "/" + parts[1];
+ else
+ fullPath = fullPath + tblName;
+
+ if (logger.isDebugEnabled()) logger.debug("createk table fullPath is " + fullPath);
+
+ String invokeret = invokeSetStoragePolicy(fs, fullPath, setDescRet.storagePolicy_ ) ;
+
+ if( invokeret != null)
+ {
+ //error handling
+ admin.close();
+ throw new IOException(invokeret);
+ }
+ }
+
+ admin.close();
return true;
}
+ private static String invokeSetStoragePolicy(final FileSystem fs, final String pathstr,
+ final String storagePolicy) {
+ String ret = null;
+ Path path = new Path(pathstr);
+ Method m = null;
+ try {
+ m = fs.getClass().getDeclaredMethod("setStoragePolicy",
+ new Class<?>[] { Path.class, String.class });
+ m.setAccessible(true);
+ } catch (NoSuchMethodException e) {
+ ret = "FileSystem doesn't support setStoragePolicy";
+ m = null;
+ } catch (SecurityException e) {
+ ret = "No access to setStoragePolicy on FileSystem from the SecurityManager";
+ m = null; // could happen on setAccessible() or getDeclaredMethod()
+ }
+ if (m != null) {
+ try {
+ m.invoke(fs, path, storagePolicy);
+ if (logger.isDebugEnabled()) {
+ logger.debug("Set storagePolicy=" + storagePolicy + " for path=" + path);
+ }
+ } catch (Exception e) {
+ logger.error("invoke set storage policy error : " + e);
--- End diff --
To preserve the exception stack, call logger.error("message", e);
---
[GitHub] trafodion pull request #1532: [TRAFODION-3026] add create option storage pol...
Posted by traflm <gi...@git.apache.org>.
Github user traflm commented on a diff in the pull request:
https://github.com/apache/trafodion/pull/1532#discussion_r193921772
--- Diff: core/sqf/src/seatrans/hbase-trx/src/main/java/org/apache/hadoop/hbase/client/transactional/TransactionManager.java ---
@@ -3226,5 +3260,65 @@ public RecoveryRequestResponse call(TrxRegionService instance) throws IOExceptio
return resultArray[0].getResultList();
}
+
+ public void setStoragePolicy(String tblName, String policy)
+ throws IOException {
+
+ int retryCount = 0;
+ int retrySleep = TM_SLEEP;
+ boolean retry = false;
+ try {
+ Table tbl = connection.getTable(TableName.valueOf(tblName));
+ String rowkey = "0";
+ CoprocessorRpcChannel channel = tbl.coprocessorService(rowkey.getBytes());
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrxRegionService.BlockingInterface service =
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrxRegionService.newBlockingStub(channel);
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyRequest.Builder request =
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyRequest.newBuilder();
+ String hbaseRoot = config.get("hbase.rootdir");
+ FileSystem fs = FileSystem.get(config);
+ //Construct the HDFS dir
+ //find out if namespace is there
+ String[] parts = tblName.split(":");
+ String namespacestr="";
+ String fullPath = hbaseRoot + "/data/" ;
+ String fullPath2 = hbaseRoot + "/data/default/";
+ if(fs.exists(new Path(fullPath2)))
+ fullPath = fullPath2;
+
+ if(parts.length >1) //have namespace
+ fullPath = fullPath + parts[0] + "/" + parts[1];
+ else
+ fullPath = fullPath + tblName;
+
+ request.setPath(fullPath);
+ request.setPolicy(policy);
+
+ do {
+ org.apache.hadoop.hbase.coprocessor.transactional.generated.TrxRegionProtos.TrafSetStoragePolicyResponse ret =
+ service.setStoragePolicy(null,request.build());
+
+ //handle result and error
+ if( ret == null)
+ {
+ LOG.error("setStoragePolicy Response ret null ");
+ }
+ else if (ret.getStatus() == false)
+ {
+ LOG.error("setStoragePolicy Response ret false: " + ret.getException());
+ throw new IOException(ret.getException());
+ }
+ if(retryCount == RETRY_ATTEMPTS)
+ {
+ throw new IOException("coprocessor not response");
+ }
+ if (retry)
+ retrySleep = retry(retrySleep);
+ } while (retry && retryCount++ < RETRY_ATTEMPTS);
+ }
+ catch (Exception e) {
+ throw new IOException(e);
+ }
+ }
--- End diff --
thanks Selva! Sorry, I should be more careful, I never understand the LOG and exception clear, will change this.
---