> On Oct. 28, 2014, 10: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. > > Veena Basavaraj wrote: > 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.
Thank you for the summary Veena! Let me address each of your points separately: > Message class - ValidationMessage and Severity field is good. Enum renames > are good Thanks! > 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 I've updated the java doc to make it more obvious. > 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 I tend to disagree on this one. Client's goal is to create or modify connection or job and hence they are calling for example saveLink [1] method to create new Link object. This method is returning back whether that operation was successfull or not (and I see your point that "Status" would be slightly better here) which is what I would expect from such method. The individual messages are actually persisted in the object that is given to us - e.g. we are changing the parameters as a side effect of the method call. I don't see a use case where client would benefit from having the ValidationResult or something similar as they would always had to apply it to the given object (Link in this case) first before reasonably using it. Hence I feel that it's correct when this application is done automatically by the API itself. 1: https://github.com/apache/sqoop/blob/sqoop2/client/src/main/java/org/apache/sqoop/client/SqoopClient.java#L323 > If we are chaning to severity please rename the test class TestStatus as well Good point, I've missed this one, renamed. > We shoudl get rid of status field on the validator. ValidationResult is the > right place to hold the status IMO. Assumption: The code base will need to get the "Worst" status for given Validator (e.g. of all the messages that it stores). We have two options how to handle that: 1) Precompute and cache the value as we are doing now 2) Every time do iteration over all message to find out what is the worst status there If you agree with my assumption and that we are expecting that this might be called more then once, I do prefer approach 1) to pre-compute and cache the value. The semantics for me is not that Validator do have Severity, the semantics is to expose operation that pretty much every use of the code will have to do. - Jarek ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27312/#review58894 ----------------------------------------------------------- On Oct. 29, 2014, midnight, Jarek Cecho wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27312/ > ----------------------------------------------------------- > > (Updated Oct. 29, 2014, midnight) > > > 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 > >
