Conversation
1. The modified RetryableClient.java file 2. The new RetryClientDemo.java example
tstirrat15
left a comment
tstirrat15
left a comment
There was a problem hiding this comment.
The bones of this seem reasonable, but I'm not sure about some of the specifics
| public void testCreateRelationship() { | ||
| // Create a relationship | ||
| Relationship relationship = createTestRelationship(); | ||
|
|
||
| // Just verify the relationship object was created correctly | ||
| assertEquals("document", relationship.getResource().getObjectType()); | ||
| assertEquals("doc1", relationship.getResource().getObjectId()); | ||
| assertEquals("viewer", relationship.getRelation()); | ||
| assertEquals("user", relationship.getSubject().getObject().getObjectType()); | ||
| assertEquals("user1", relationship.getSubject().getObject().getObjectId()); | ||
| } |
There was a problem hiding this comment.
These tests construct a retryable client but they don't actually exercise any of the behavior 🤔
| /** | ||
| * Get the schema service client. | ||
| * | ||
| * @return The schema service client | ||
| */ | ||
| public SchemaServiceGrpc.SchemaServiceBlockingStub schemaService() { | ||
| return schemaService; | ||
| } |
There was a problem hiding this comment.
I don't know if this is idiomatic java but this pattern annoys me. I'd rather just make these public members.
| this.schemaService = SchemaServiceGrpc.newBlockingStub(channel) | ||
| .withCallCredentials(credentials); | ||
| this.permissionsService = PermissionsServiceGrpc.newBlockingStub(channel) | ||
| .withCallCredentials(credentials); | ||
| this.asyncPermissionsService = PermissionsServiceGrpc.newStub(channel) | ||
| .withCallCredentials(credentials); | ||
| this.experimentalService = ExperimentalServiceGrpc.newBlockingStub(channel) | ||
| .withCallCredentials(credentials); | ||
| this.asyncExperimentalService = ExperimentalServiceGrpc.newStub(channel) | ||
| .withCallCredentials(credentials); | ||
| } |
There was a problem hiding this comment.
This part is strange to me - we don't actually provide a client otherwise, so exposing services that aren't relevant to retryable bulk import is strange to me.
It'd be less strange if we had a client definition 🙃
It also mixes blocking and non-blocking stubs which isn't obvious behavior. I think I'd rather cut this down to just the service required for retryable bulk import.
| if (conflictStrategy == ConflictStrategy.SKIP) { | ||
| // Skip conflicts - return success | ||
| logger.log(Level.INFO, "ALREADY_EXISTS detected with SKIP strategy - returning success"); | ||
| return relationships.size(); |
There was a problem hiding this comment.
What does this actually return? Is this function supposed to return void or is it implicitly typed?
|
AI slop is bad. I'll try again. Closing. |
2 participants
Adds RetryableClient client that handles retrying bulk relationship imports with different conflict resolution strategies (FAIL, SKIP, TOUCH).
Adds a demo application (RetryClientDemo.java) that retry bulk import with each conflict strategy.