> On May 4, 2018, 12:04 p.m., David Radley wrote: > > common/src/main/java/org/apache/atlas/repository/Constants.java > > Lines 80 (patched) > > <https://reviews.apache.org/r/66949/diff/1/?file=2016731#file2016731line80> > > > > I suggest a comment here to describe how an Atlas developer should > > understand this field. > > > > A concern I have, is that if the users go through the om aPI then the > > entities with a non-null homId will never be updated - apart from as part > > of the asynchronous cnotification process from anotehr server. The Atlas > > developer using the local Atlas API, needs to be aware not to change the > > entities and relationships with a none null homeId. Ideally we should > > police this.
I can certainly add a comment to say what the field is for. I see your point about the policing of updates but I might suggest that should be a separate JIRA. > On May 4, 2018, 12:04 p.m., David Radley wrote: > > intg/src/main/java/org/apache/atlas/type/AtlasTypeUtil.java > > Lines 61 (patched) > > <https://reviews.apache.org/r/66949/diff/1/?file=2016734#file2016734line61> > > > > I am wondering why you are adding a Logger and not using it om this > > change. I had debug statements in the code during development and testing; they have been removed for brevity but I forgot to clear out the logger. > On May 4, 2018, 12:04 p.m., David Radley wrote: > > intg/src/main/java/org/apache/atlas/type/AtlasTypeUtil.java > > Line 384 (original), 389 (patched) > > <https://reviews.apache.org/r/66949/diff/1/?file=2016734#file2016734line389> > > > > I suggest adding a comment here, around why you are checking for '-' as > > the first character. > > > > What happens if another repository creates an identifier starting with > > a '-' character? > > > > to cope with theis case - I am suggest that the logic should check for > > a null homeId and a guid starting character of '-' (or null) we should say > > the guid is not assigned. I like that - I wrote the GUID handling change before adding the homeId system attribute. We could (now) include a check for homeId (not null) in the GUID validity checking. > On May 4, 2018, 12:04 p.m., David Radley wrote: > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java > > Line 709 (original), 709 (patched) > > <https://reviews.apache.org/r/66949/diff/1/?file=2016736#file2016736line709> > > > > why are we leaving in commented out code No valid reason - I wanted to highlight the effect of the change by having the 'before' and 'after' - but if the patch is seen as generally OK I should remove it. - Graham ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66949/#review202433 ----------------------------------------------------------- On May 4, 2018, 10:55 a.m., Graham Wallis wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66949/ > ----------------------------------------------------------- > > (Updated May 4, 2018, 10:55 a.m.) > > > Review request for atlas, Ashutosh Mestry and Madhan Neethiraj. > > > Repository: atlas > > > Description > ------- > > ATLAS-2523: Changes to accept external GUIDs and manage homeIds > > > Diffs > ----- > > common/src/main/java/org/apache/atlas/repository/Constants.java > c570be2ccbc41a426b0f093b4a33163092223f2f > intg/src/main/java/org/apache/atlas/model/instance/AtlasEntity.java > e6a7f19633a1642f6b415db99e20c0641df9463b > intg/src/main/java/org/apache/atlas/model/instance/AtlasRelationship.java > d04daa5d283fdba90b917f38eba6c937021352df > intg/src/main/java/org/apache/atlas/type/AtlasTypeUtil.java > 1b6af94f24eb7d99953f72815167c868a9bd9efd > repository/src/main/java/org/apache/atlas/repository/graph/GraphHelper.java > edf10da9533803234342dc3313ea1024c5e7030f > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasRelationshipStoreV1.java > 9fcba6ddaa1b74b2c81b980bcb455b614f4a4ed7 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphMapper.java > d51adad80e133d636ef84883d395126ae0150af5 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityGraphRetriever.java > 3b9a287f6986ff6bca26fc20b2f94ec1700dbe1e > > > Diff: https://reviews.apache.org/r/66949/diff/1/ > > > Testing > ------- > > Functional testing of these changes to save reference copies of entities and > relationships > > > Thanks, > > Graham Wallis > >
