Copilot commented on code in PR #426:
URL: https://github.com/apache/atlas/pull/426#discussion_r2277964419
##########
repository/src/main/java/org/apache/atlas/repository/patches/UniqueAttributePatch.java:
##########
@@ -160,12 +158,11 @@ private void createIndexForUniqueAttributes(String
typeName, Collection<AtlasAtt
AtlasAttributeDef.IndexType.STRING.equals(attribute.getIndexType()));
}
- getIndexer().commit(management);
- getGraph().commit();
-
LOG.info("Unique attributes: type: {}: Registered!", typeName);
- } catch (IndexException e) {
+ } catch (Exception e) {
LOG.error("Error creating index: type: {}", typeName, e);
+ } finally {
+ getGraph().commit();
}
Review Comment:
The finally block is now executing after the try-with-resources has already
closed the management instance. This could lead to unexpected behavior since
the management transaction may already be committed or rolled back. Consider
moving the graph.commit() call inside the try block or removing the finally
block entirely if the management handles the transaction lifecycle.
```suggestion
}
getGraph().commit();
```
##########
repository/src/main/java/org/apache/atlas/repository/graph/GraphBackedSearchIndexer.java:
##########
@@ -289,22 +284,11 @@ public void onChange(ChangedTypeDefs changedTypeDefs)
throws AtlasBaseException
createEdgeLabels(management, changedTypeDefs.getCreatedTypeDefs());
createEdgeLabels(management, changedTypeDefs.getUpdatedTypeDefs());
- isRollbackNeeded = false;
-
- //Commit indexes
- commit(management);
- } catch (RepositoryException | IndexException e) {
+ management.setIsSuccess(true);
+ } catch (Exception e) {
LOG.error("Failed to update indexes for changed typedefs", e);
-
- isRollbackNeeded = false;
-
- attemptRollback(changedTypeDefs, management);
} finally {
- if (isRollbackNeeded) {
- LOG.warn("onChange({}): was not committed. Rolling back...",
changedTypeDefs);
-
- attemptRollback(changedTypeDefs, management);
- }
+ recomputeIndexedKeys = true;
Review Comment:
[nitpick] The recomputeIndexedKeys flag is set in the finally block, but
it's also set conditionally in other parts of the code. This could lead to
inconsistent state management. Consider consolidating the flag management logic
to ensure predictable behavior.
##########
graphdb/api/src/main/java/org/apache/atlas/repository/graphdb/AtlasGraphManagement.java:
##########
@@ -24,7 +24,7 @@
* Management interface for a graph.
*
*/
-public interface AtlasGraphManagement {
+public interface AtlasGraphManagement extends AutoCloseable {
/**
Review Comment:
The AutoCloseable interface's close() method throws Exception, but the
existing interface methods don't declare checked exceptions. This creates an
inconsistency in the API design. Consider overriding close() to not throw
checked exceptions or document the expected exception handling behavior.
```suggestion
/**
* Closes the management interface. Does not throw checked exceptions.
*/
@Override
void close();
/**
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
