> On Oct. 28, 2014, 3:08 p.m., Veena Basavaraj wrote: > > can you elaborate more on this rename. > > > > Usually status is pretty common > > > > http://enrich-ege.sourceforge.net/multiprojects/ege-api/apidocs/pl/psnc/dl/ege/types/ValidationResult.Status.html > > > > the error levels rename looks good though. > > Jarek Cecho wrote: > I feel that the "Severity" is much representative of the what is the > intention here. It's probably a more subjective thing though. > > More objective (but weak) point is that we already have different Status > [1] in the system for submissions so I also wanted to change the word to > separate those two things. > > [1]: > https://github.com/apache/sqoop/blob/sqoop2/common/src/main/java/org/apache/sqoop/submission/SubmissionStatus.java > > Veena Basavaraj wrote: > So here is the jist of the validation story as I understand from the > latest code. > > > 1. AbstractValidator has a method call status. Not sure what the > intention is. > > /** > * Overall status of the validation. > */ > private Status status; > > > 2. ConfigValidationRunner is pretty cool too since it is like a util > class we use to do the actual config validations > > 3. ConfigValidationResult class is also cool since it holds the result of > the validation. But on the validation result, overall status seems a better > option. Status of the validation operation. > > > 4. Severity on the validation message seems very apt to me and the enum > also makes sense. > > > Aside, > > Would be nice to have a ticket to update the docs on the validation > execution for configs.
I will give another shot at summarizing 1. Message class - ValidationMessage and Severity field is good. Enum renames are good 2. ConfigValidationRunner validate returns a ValidationResult that holds the aggregated severity, here the aggregate just feels like the overall status, but may be updating java docs is fine and we can live with Severity 3. The client apis should be consistent and return the same ValidatioMessage object instead of this Status. So the Message has TEXT and SEVERITY in it 4. If we are chaning to severity please rename the test class TestStatus as well 5. We shoudl get rid of status field on the validator. ValidationResult is the right place to hold the status IMO. - Veena ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27312/#review58894 ----------------------------------------------------------- On Oct. 28, 2014, 5 p.m., Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27312/ > ----------------------------------------------------------- > > (Updated Oct. 28, 2014, 5 p.m.) > > > Review request for Sqoop. > > > Bugs: SQOOP-1469 > https://issues.apache.org/jira/browse/SQOOP-1469 > > > Repository: sqoop-sqoop2 > > > Description > ------- > > This page is getting crazy big, but only contains the following change(s): > > * Renamed class Status to Severity > * Renamed "FINE" to "OK" > * Renamed "ACCEPTABLE" to "WARNING" > * Renamed "UNACCEPTABLE" to "ERROR" > > Which were suggestions that were there for quite some time. > > > Diffs > ----- > > client/src/main/java/org/apache/sqoop/client/SqoopClient.java 33a0c3c > common/src/main/java/org/apache/sqoop/json/ValidationResultBean.java > 4b3fc37 > common/src/main/java/org/apache/sqoop/model/MValidatedElement.java c0d678a > > common/src/main/java/org/apache/sqoop/validation/ConfigValidationResult.java > 4c4d123 > > common/src/main/java/org/apache/sqoop/validation/ConfigValidationRunner.java > 8c66b3d > common/src/main/java/org/apache/sqoop/validation/Message.java 3361b6f > common/src/main/java/org/apache/sqoop/validation/Severity.java PRE-CREATION > common/src/main/java/org/apache/sqoop/validation/Status.java 5a546bf > > common/src/main/java/org/apache/sqoop/validation/validators/AbstractValidator.java > 9dbb44c > > common/src/main/java/org/apache/sqoop/validation/validators/ClassAvailable.java > 52a8fdd > common/src/main/java/org/apache/sqoop/validation/validators/Contains.java > 8920c72 > common/src/main/java/org/apache/sqoop/validation/validators/NotEmpty.java > 248a2fa > common/src/main/java/org/apache/sqoop/validation/validators/NotNull.java > 93b5fa7 > > common/src/main/java/org/apache/sqoop/validation/validators/NullOrContains.java > 9d11a2e > common/src/main/java/org/apache/sqoop/validation/validators/StartsWith.java > 808e1f7 > common/src/test/java/org/apache/sqoop/json/TestValidationResultBean.java > bdbad72 > common/src/test/java/org/apache/sqoop/model/TestMValidatedElement.java > a75a7cd > common/src/test/java/org/apache/sqoop/validation/TestStatus.java 99d8076 > common/src/test/java/org/apache/sqoop/validation/TestValidationRunner.java > 579d1c5 > > common/src/test/java/org/apache/sqoop/validation/validators/TestClassAvailable.java > 3a15274 > > common/src/test/java/org/apache/sqoop/validation/validators/TestContains.java > e63a69a > > common/src/test/java/org/apache/sqoop/validation/validators/TestNotEmpty.java > 5c9169a > > common/src/test/java/org/apache/sqoop/validation/validators/TestNotNull.java > 91e5398 > > common/src/test/java/org/apache/sqoop/validation/validators/TestNullOrContains.java > 88347eb > > common/src/test/java/org/apache/sqoop/validation/validators/TestStartsWith.java > 8c4f9e0 > > common/src/test/java/org/apache/sqoop/validation/validators/TestValidator.java > 3f60ee6 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/FromJobConfig.java > 12ceb21 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/LinkConfig.java > be86855 > > connector/connector-generic-jdbc/src/main/java/org/apache/sqoop/connector/jdbc/configuration/ToJobConfig.java > 2428601 > > connector/connector-generic-jdbc/src/test/java/org/apache/sqoop/connector/jdbc/TestToInitializer.java > 243de01 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/configuration/LinkConfig.java > c63f8a8 > > connector/connector-hdfs/src/main/java/org/apache/sqoop/connector/hdfs/configuration/ToJobConfig.java > 7fb1f74 > > connector/connector-hdfs/src/test/java/org/apache/sqoop/connector/hdfs/TestLinkConfig.java > 176d0df > core/src/main/java/org/apache/sqoop/repository/Repository.java 79742b9 > core/src/test/java/org/apache/sqoop/repository/TestJdbcRepository.java > a1e734e > server/src/main/java/org/apache/sqoop/handler/JobRequestHandler.java > 5547988 > server/src/main/java/org/apache/sqoop/handler/LinkRequestHandler.java > 35a9635 > shell/src/main/java/org/apache/sqoop/shell/CloneJobFunction.java 8188831 > shell/src/main/java/org/apache/sqoop/shell/CloneLinkFunction.java c1a4f55 > shell/src/main/java/org/apache/sqoop/shell/CreateJobFunction.java 6f136b2 > shell/src/main/java/org/apache/sqoop/shell/CreateLinkFunction.java ce9988f > shell/src/main/java/org/apache/sqoop/shell/DeleteJobFunction.java da26153 > shell/src/main/java/org/apache/sqoop/shell/DeleteLinkFunction.java c81917e > shell/src/main/java/org/apache/sqoop/shell/DisableJobFunction.java 9e46804 > shell/src/main/java/org/apache/sqoop/shell/DisableLinkFunction.java 6b85292 > shell/src/main/java/org/apache/sqoop/shell/EnableJobFunction.java 8575a84 > shell/src/main/java/org/apache/sqoop/shell/EnableLinkFunction.java 1dd30e8 > shell/src/main/java/org/apache/sqoop/shell/SetOptionFunction.java ccc067f > shell/src/main/java/org/apache/sqoop/shell/SetServerFunction.java 84df281 > shell/src/main/java/org/apache/sqoop/shell/ShowConnectorFunction.java > d605457 > shell/src/main/java/org/apache/sqoop/shell/ShowDriverFunction.java 080792b > shell/src/main/java/org/apache/sqoop/shell/ShowJobFunction.java 0640283 > shell/src/main/java/org/apache/sqoop/shell/ShowLinkFunction.java f4eae33 > shell/src/main/java/org/apache/sqoop/shell/ShowOptionFunction.java 4bb0cab > shell/src/main/java/org/apache/sqoop/shell/ShowServerFunction.java 67eb6a6 > shell/src/main/java/org/apache/sqoop/shell/ShowSubmissionFunction.java > 2d00b88 > shell/src/main/java/org/apache/sqoop/shell/ShowVersionFunction.java 3b4bb3f > shell/src/main/java/org/apache/sqoop/shell/StartJobFunction.java 4363f05 > shell/src/main/java/org/apache/sqoop/shell/StatusJobFunction.java fb83af3 > shell/src/main/java/org/apache/sqoop/shell/StopJobFunction.java 790c522 > shell/src/main/java/org/apache/sqoop/shell/UpdateJobFunction.java dd075d7 > shell/src/main/java/org/apache/sqoop/shell/UpdateLinkFunction.java 176833a > shell/src/main/java/org/apache/sqoop/shell/utils/ConfigDisplayer.java > dea271a > shell/src/main/java/org/apache/sqoop/shell/utils/ConfigFiller.java c61d33b > shell/src/main/resources/shell-resource.properties 0e63c50 > test/src/main/java/org/apache/sqoop/test/testcases/ConnectorTestCase.java > af31769 > tools/src/main/java/org/apache/sqoop/tools/tool/RepositoryLoadTool.java > 64b08fc > > Diff: https://reviews.apache.org/r/27312/diff/ > > > Testing > ------- > > Unit tests are passing. > > > Thanks, > > Jarek Cecho > >
