[ https://issues.apache.org/jira/browse/HDFS-13245?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16468533#comment-16468533 ]
Yiqun Lin edited comment on HDFS-13245 at 5/9/18 8:17 AM: ---------------------------------------------------------- Thanks [~yiran] for updating the patch. Some comments from me: *StateStoreSQLImpl .java* 1.In method {{insertRecord}} and {{updateRecord}}, {code:java} } catch (UnsupportedEncodingException e) { + LOG.error("Cannot insert record for \"{}\", sql: \"{}\".", + tableName, sql, e); + } finally { {code} 'Cannot insert record for' can be updated to ''Unsupported encoding error for' 2. {code:java} LOG.error("Cannot auto create table \"{}\"" + + ". type is \"{}\", sql: \"{}\".", dbType, tableName, sql, e); {code} {{. type}} should be {{, type}} *hdfs-rbf-default.xml.java* 1. In description of {{dfs.federation.router.store.sql.jdbc.class}}, we miss the JDBC class for HSSQLDB. 2.'Database connection url. such as' updates to {{Database connection url, such as}} 3.'if value less than 1, use 1.' updates to {{If value less than 1, use 1.}} 4. For the setting {{dfs.federation.router.store.sql.auto.create.schema.enabled}}, not so sure if I understanding correctly. Would you add more description for this? But from my perspective, this setting seems no needed. 5. The description of {{dfs.federation.router.store.driver.class}} can be updated, we can add DBMS class there. *TestStateStore.java* Also not sure this new test is really needed. I mean {{TestStateStoreDriverBase}} may have cover these cases, for example driver connection, etc. *MockStateStoreSQLImpl.java* Why we mock a new StateStoreSQLImpl that only testing for MSSQLServer? I hardly see any difference between MockStateStoreSQLImpl and StateStoreSQLImpl. was (Author: linyiqun): Thanks [~yiran] for updating the patch. Some comments from me: *StateStoreSQLImpl .java* 1.In method {{insertRecord}} and {{updateRecord}}, {code:java} } catch (UnsupportedEncodingException e) { + LOG.error("Cannot insert record for \"{}\", sql: \"{}\".", + tableName, sql, e); + } finally { {code} 'Cannot insert record for' can be updated to ''Unsupported encoding error for' 2. {code:java} LOG.error("Cannot auto create table \"{}\"" + + ". type is \"{}\", sql: \"{}\".", dbType, tableName, sql, e); {code} {{. type}} should be {{, type}} *hdfs-rbf-default.xml.java* 1. In description of {{dfs.federation.router.store.sql.jdbc.class}}, we miss the JDBC class for HSSQLDB. 2.'Database connection url. such as' updates to {{Database connection url, such as}} 3.'if value less than 1, use 1.' updates to {{If value less than 1, use 1.}} 4. For the setting {{dfs.federation.router.store.sql.auto.create.schema.enabled}}, not so sure if I understanding correctly. Would you add more description for this? But from my perspective, this setting seems no needed. *TestStateStore.java* Also not sure this new test is really needed. I mean {{TestStateStoreDriverBase}} may have cover these cases, for example driver connection, etc. *MockStateStoreSQLImpl.java* Why we mock a new StateStoreSQLImpl that only testing for MSSQLServer? I hardly see any difference between MockStateStoreSQLImpl and StateStoreSQLImpl. > RBF: State store DBMS implementation > ------------------------------------ > > Key: HDFS-13245 > URL: https://issues.apache.org/jira/browse/HDFS-13245 > Project: Hadoop HDFS > Issue Type: Sub-task > Components: hdfs > Reporter: maobaolong > Assignee: Yiran Wu > Priority: Major > Attachments: HDFS-13245.001.patch, HDFS-13245.002.patch, > HDFS-13245.003.patch, HDFS-13245.004.patch, HDFS-13245.005.patch, > HDFS-13245.006.patch, HDFS-13245.007.patch, HDFS-13245.008.patch, > HDFS-13245.009.patch, HDFS-13245.010.patch > > > Add a DBMS implementation for the State Store. -- This message was sent by Atlassian JIRA (v7.6.3#76005) --------------------------------------------------------------------- To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org