----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25074/#review51592 -----------------------------------------------------------
First pass! Good start! common/src/main/java/org/apache/sqoop/model/FormUtils.java <https://reviews.apache.org/r/25074/#comment90027> Kill space common/src/main/java/org/apache/sqoop/model/FormUtils.java <https://reviews.apache.org/r/25074/#comment90028> No need for newline. common/src/main/java/org/apache/sqoop/model/FormUtils.java <https://reviews.apache.org/r/25074/#comment90039> You should be able to replace this with: getFieldFromName(Class<?> klass, name) { try { klass.getDeclaredField(name) } catch (...) { <previous logic> } } Also, I'd try to keep the nomenclature here more generic so that we can reuse this code for Input's in the future if we need to. common/src/main/java/org/apache/sqoop/model/FormUtils.java <https://reviews.apache.org/r/25074/#comment90030> This works! I'm wondering if we can simply have a lookup table somewhere! common/src/main/java/org/apache/sqoop/model/FormUtils.java <https://reviews.apache.org/r/25074/#comment90040> See note above about getting field from name. common/src/main/java/org/apache/sqoop/model/FormUtils.java <https://reviews.apache.org/r/25074/#comment90031> These don't need to be on different lines. common/src/main/java/org/apache/sqoop/model/FormUtils.java <https://reviews.apache.org/r/25074/#comment90032> This doesn't seem to need to be on different lines. common/src/main/java/org/apache/sqoop/model/FormUtils.java <https://reviews.apache.org/r/25074/#comment90034> kill space common/src/main/java/org/apache/sqoop/model/FormUtils.java <https://reviews.apache.org/r/25074/#comment90035> Kill space common/src/main/java/org/apache/sqoop/model/FormUtils.java <https://reviews.apache.org/r/25074/#comment90036> I think this could be made public and be a little bit more robust. Some of the logic could be encapsulated in this method. For instance, you could pass a Field object and the associated annotation. Then choose which name to use. IE: public static String getFormName(Field member, Form annotation) { if (StringUtils.empty(annotation.getName())) { return member.getName(); } else { checkForValidFormName(...); return annotation.getName(); } } We might be able to use this code in the future for Input's if we want to! common/src/main/java/org/apache/sqoop/model/FormUtils.java <https://reviews.apache.org/r/25074/#comment90037> void instead of boolean? common/src/main/java/org/apache/sqoop/model/FormUtils.java <https://reviews.apache.org/r/25074/#comment90038> +1! common/src/test/java/org/apache/sqoop/model/TestFormUtils.java <https://reviews.apache.org/r/25074/#comment90041> Is this move necessary? connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ConnectionConfiguration.java <https://reviews.apache.org/r/25074/#comment90024> Let's not change this so that the forms are upgradeable. connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ExportJobConfiguration.java <https://reviews.apache.org/r/25074/#comment90023> Let's not change this so that the forms are upgradeable. connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ImportJobConfiguration.java <https://reviews.apache.org/r/25074/#comment90022> Let's not change this so that the forms are upgradeable. core/src/main/java/org/apache/sqoop/framework/configuration/ConnectionConfiguration.java <https://reviews.apache.org/r/25074/#comment90021> Let's not change this so that the forms are upgradeable. core/src/main/java/org/apache/sqoop/framework/configuration/ExportJobConfiguration.java <https://reviews.apache.org/r/25074/#comment90020> Let's not change this so that the forms are upgradeable. core/src/main/java/org/apache/sqoop/framework/configuration/ImportJobConfiguration.java <https://reviews.apache.org/r/25074/#comment90019> Let's not change this so that the forms are upgradeable. - Abraham Elmahrek On Aug. 26, 2014, 9:40 p.m., Veena Basavaraj wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25074/ > ----------------------------------------------------------- > > (Updated Aug. 26, 2014, 9:40 p.m.) > > > Review request for Sqoop, Abraham Elmahrek, Gwen Shapira, and Jarek Cecho. > > > Bugs: SQOOP-1436 > https://issues.apache.org/jira/browse/SQOOP-1436 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > https://issues.apache.org/jira/browse/SQOOP-1436 > As discussed in the SQOOP-1436, this feature gives the ability to specifiy a > custom form name. It is optional, if given it is persisted in the metadata > repo. Methods that convert the form fields to JSON have also been modified to > read the custom form name attribute. basic validation for uniqueness, length > and pattern have been added > > Existing unit tests modified to support the custom form name attribute > unit tests added for validation cases > > Note: there are formatting changes since I used the mandated eclipse > formatter.xml in the IDE > > > Diffs > ----- > > common/src/main/java/org/apache/sqoop/model/Form.java 4321582 > common/src/main/java/org/apache/sqoop/model/FormUtils.java 27db8af > common/src/main/java/org/apache/sqoop/model/ModelError.java 1f466fe > common/src/test/java/org/apache/sqoop/model/TestFormUtils.java 08dfa7b > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ConnectionConfiguration.java > 6061600 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ExportJobConfiguration.java > f2b2d65 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ImportJobConfiguration.java > f3c1d13 > > core/src/main/java/org/apache/sqoop/framework/configuration/ConnectionConfiguration.java > 830606a > > core/src/main/java/org/apache/sqoop/framework/configuration/ExportJobConfiguration.java > 6665429 > > core/src/main/java/org/apache/sqoop/framework/configuration/ImportJobConfiguration.java > 2a35eb9 > > Diff: https://reviews.apache.org/r/25074/diff/ > > > Testing > ------- > > mvn test and integration tests pass. > > > Thanks, > > Veena Basavaraj > >
