Repository navigation
Conversation
| }) | ||
| }) | ||
| .await | ||
| .map_err(|e| tonic::Status::internal(format!("Reconciliation task panicked: {e}")))?; |
There was a problem hiding this comment.
I don't think it panicked, it just returned an error? I might not actually understand what's going on under the hood here though.
There was a problem hiding this comment.
For normal errors (like CAS conflict, if the database table is not there, database connection fails, etc), that is caught on line 104 with the match reconcile_result line. The line
.map_err(|e| tonic::Status::internal(format!("Reconciliation task panicked: {e}")))?;
is only called when the thread panics/crashes, or the tokio runtime has an error. It is correctly characterized as a panic.
| // performs a 3-way view merge, creates a new merge operation, and eventually invokes | ||
| // the `Store` trait (`crate::store::Store::update_op_heads` via `ServerOpHeadsStore`) | ||
| // with strict exact CAS verification. | ||
| let repo = repo_loader.load_at_head().await.map_err(|e| e.to_string())?; |
There was a problem hiding this comment.
It's a bit surprising that we're creating a full RepoLoader from scratch here just to call this one method. I don't necessarily disagree with it -- reimplementing the logic seems worse, but I guess I'm a little surprised that there's not already a RepoLoader around that you can use. If there's not one already made somewhere somehow, then I'm also surprised that there's not already a helper function that reduces the boilerplate here :D If this is the only place you need to do this, then i don't think you need that helper function, but I thought RepoLoader was a pretty fundamental thing in jj.
There was a problem hiding this comment.
There is no RepoLoader I can use, since prior to this, the server did not have a dependence on jj-lib. While RepoLoader is a fundamental part of jj, it exists only in jj-lib, which was not a dependency of the server before this.
RepoLoader only needs to be created on the server side for the op heads reconcilation, and this is the only place it is instantiated.
There was a problem hiding this comment.
I made a similar comment on the previous PR in the chain. jj does not yet have server-friendly APIs. The meat is in RepoLoader::merge_operations. I am 99% sure the RepoLoader's settings are never used in load_at_head, so why does it even need that? There is no good way to simplify this in the short-term. In the long run I think upstream jj will have to be refactored to extract simpler server-side APIs. For now this looks good to me.
There was a problem hiding this comment.
Yeah, I think its annoying that we still need to pass in settings to make this work. I've mentioned this in a future work roadmap document (shared internally)
cd4f9a1 to
fed2510
Compare
| // performs a 3-way view merge, creates a new merge operation, and eventually invokes | ||
| // the `Store` trait (`crate::store::Store::update_op_heads` via `ServerOpHeadsStore`) | ||
| // with strict exact CAS verification. | ||
| let repo = repo_loader.load_at_head().await.map_err(|e| e.to_string())?; |
There was a problem hiding this comment.
I made a similar comment on the previous PR in the chain. jj does not yet have server-friendly APIs. The meat is in RepoLoader::merge_operations. I am 99% sure the RepoLoader's settings are never used in load_at_head, so why does it even need that? There is no good way to simplify this in the short-term. In the long run I think upstream jj will have to be refactored to extract simpler server-side APIs. For now this looks good to me.
| let store_clone = store.clone(); | ||
| let repo_id_owned = repo_id.to_string(); | ||
|
|
||
| // Offload reconciliation to Tokio's blocking thread pool (`spawn_blocking`). |
There was a problem hiding this comment.
I am surprised to see this here and I don't have much detailed knowledge of Tokio's model and why you would want spawn_block here. I would think you don't need it. On Google's jj server we use scaffolding and I suppose that takes care of this sort of thing. Are you sure we need this?
There was a problem hiding this comment.
spawn_block here is needed, since jj-lib load_at_head is not 'Send' ... tonic requires that, so we need to run this on a separate thread.
Google JJ server eventually calls Pollster block_on() for op head reconciliation, which is the same synchronous functionality. I've added a comment explaining exactly why above this.
There was a problem hiding this comment.
Look into upstream change, adding a new function into RepoLoader that returns 'Send', so we can just call await on this, instead of having to do tokio spawn blocking stuff
fed2510 to
ba02f90
Compare
Implement the core logic and function call to do the op head reconciliation on the server. The reconcile_repo_op_heads, which is called by the reconcile_op_heads RPC call, handles the core logic of the jj-lib function call that reconciles. A new RepoLoader is instantiated, which calls load_at_head, which calls resolve_op_heads (a standalone function in jj-lib). This then uses the provided jj adapters to perform the reconciliation algorithm, producing a new jj operation. This new jj-operation is returned from the reconcile_repo_op_heads function into the reconcile_op_heads RPC handler. The jj adapters need to be created every time and cannot be persisted across the entire server since it takes repo_id on construction, which will be different for every reconciliation call. There can be some future performance improvements for this, a TODO comment above is added, but leave it like this for the POC. By way of the server-side jj adapter implementation, the Store trait for update_op_heads_compare_and_swap is invoked and the database transaction is invoked. If this transaction fails, the function retries MAX_RETRIES times, with exponential backoff + randomized jitter every time, hoping to prevent further collisions. If the transaction fails MAX_RETRIES times, it stops the op head reconciliation, and surfaces an error to the client, which is the intended jj-lib behavior. RECONCILER_ENV is used to cache jj-lib UserSettings and MergeOptions configs for faster instantiations when passing into RepoLoader.
ba02f90 to
287827a
Compare
server: add core engine for op head reconciliation
Implement the core logic and function call to do the op head reconciliation on the server. The reconcile_repo_op_heads, which is called by the reconcile_op_heads RPC call, handles the core logic of the jj-lib function call that reconciles.
A new RepoLoader is instantiated, which calls load_at_head, which calls resolve_op_heads (a standalone function in jj-lib). This then uses the provided jj adapters to perform the reconciliation algorithm, producing a new jj operation. This new jj-operation is returned from the reconcile_repo_op_heads function into the reconcile_op_heads RPC handler. The jj adapters need to be created every time and cannot be persisted across the entire server since it takes repo_id on construction, which will be different for every reconciliation call. There can be some future performance improvements for this, a TODO comment above is added, but leave it like this for the POC.
By way of the server-side jj adapter implementation, the Store trait for update_op_heads_compare_and_swap is invoked and the database transaction is invoked. If this transaction fails, the function retries MAX_RETRIES times, with exponential backoff + randomized jitter every time, hoping to prevent further collisions. If the transaction fails MAX_RETRIES times, it stops the op head reconciliation, and surfaces an error to the client, which is the intended jj-lib behavior.
RECONCILER_ENV is used to cache jj-lib UserSettings and MergeOptions configs for faster instantiations when passing into RepoLoader.