----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62175/#review186223 -----------------------------------------------------------
repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java Lines 109 (patched) <https://reviews.apache.org/r/62175/#comment262701> I suggest that we should not pass through wording to a message. The "null/empty guid" should be part of the text of a new error message. We then get more unique messages to aid debugging and well as easing globalization if we decide to do that later. repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java Lines 152 (patched) <https://reviews.apache.org/r/62175/#comment262702> samr as previous comment repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java Lines 161 (patched) <https://reviews.apache.org/r/62175/#comment262708> If this edge is soft deleted, then the deleterelationships ignores the edge. We should throw an error here, if there is an attempt to delete an already deleted relationship. repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java Lines 245 (patched) <https://reviews.apache.org/r/62175/#comment262709> Ideally we should only set the properties in the edge if they are specified in the request i.e. all the ones that relationship.getAttribute(attrName); has a value. Unfortunately it doesn ot look like the code differentiates between null values and unspecified attributes. repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java Lines 254 (patched) <https://reviews.apache.org/r/62175/#comment262703> Asis you do not need the ret variable. I suggest keeping it and adding tracing putting out the ret. repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java Line 252 (original), 302 (patched) <https://reviews.apache.org/r/62175/#comment262710> I am wondering why we are deleting the tracing repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java Line 288 (original) <https://reviews.apache.org/r/62175/#comment262711> why are we deleting the tracing? repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java Line 356 (original), 405 (patched) <https://reviews.apache.org/r/62175/#comment262704> Shouldn't this method re-use any soft deleted edge? repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java Line 360 (original), 408 (patched) <https://reviews.apache.org/r/62175/#comment262707> It does not look like ret can be null here? repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java Line 363 (original), 411 (patched) <https://reviews.apache.org/r/62175/#comment262706> what about relationship attributes - shouldn't we overwrite them here. repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java Lines 429 (patched) <https://reviews.apache.org/r/62175/#comment262705> This looks strange. You check for one value and set the otehr. If this is what you want to do, please add a comment to show your thinking. - David Radley On Sept. 7, 2017, 10:16 p.m., Sarath Subramanian wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62175/ > ----------------------------------------------------------- > > (Updated Sept. 7, 2017, 10:16 p.m.) > > > Review request for atlas. > > > Bugs: ATLAS-2119 > https://issues.apache.org/jira/browse/ATLAS-2119 > > > Repository: atlas > > > Description > ------- > > * Implement update and delete relationship in RelationshipREST > * Add propagateTags information to each relationship instance > > > Diffs > ----- > > intg/src/main/java/org/apache/atlas/model/instance/AtlasRelationship.java > ec6161c0 > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > 020dd457 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasGraphUtilsV1.java > 227f7cd1 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java > 9b273193 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/DeleteHandlerV1.java > b0940f67 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java > f6aa7bbf > > repository/src/test/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1Test.java > 94cc5b93 > > > Diff: https://reviews.apache.org/r/62175/diff/1/ > > > Testing > ------- > > validated using POSTMAN Rest client. > > Integration/Unit Tests in progress > > > Thanks, > > Sarath Subramanian > >
