Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interface refactor #1248

Merged
merged 19 commits into from
Mar 7, 2024
Merged

Interface refactor #1248

merged 19 commits into from
Mar 7, 2024

Conversation

slinkydeveloper
Copy link
Contributor

No description provided.

Copy link

github-actions bot commented Feb 29, 2024

Test Results

 94 files   94 suites   3m 36s ⏱️
 78 tests  42 ✅ 36 💤 0 ❌
206 runs  111 ✅ 95 💤 0 ❌

Results for commit 8f125ea.

♻️ This comment has been updated with latest results.

@slinkydeveloper
Copy link
Contributor Author

I've tested the branch with restatedev/e2e#272 and restatedev/sdk-java#236 and e2e tests are passing!

@slinkydeveloper
Copy link
Contributor Author

Checked after the rebase, everything works

This refactor includes using latest tower version, remove manual CORS handling code, and reorganize the handler code.
Also includes the first bits to support awakeables resolution.
@slinkydeveloper slinkydeveloper merged commit 681cd8f into main Mar 7, 2024
6 checks passed
@slinkydeveloper slinkydeveloper deleted the interface-refactor branch March 7, 2024 08:31
Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't manage to review this PR fully before it was merged. Will stop now and just post my minor comments I had so far.

Comment on lines +54 to 55
let services = client.get_components().await?.into_body().await?.components;
for svc in services {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: services -> components, svc -> component

Comment on lines +99 to 100
for svc in deployment.components {
let Some(latest_svc) = latest_services.get(&svc.name) else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Renaming (service -> component) seems to be not complete. Won't mention it anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm tracking this here #1225

Cell::new(&method.output_type),
Cell::new(&handler.name),
Cell::new(handler.input_description.as_deref().unwrap_or("any")),
Cell::new(handler.output_description.as_deref().unwrap_or("any")),
]);
}
table
}

pub fn create_service_methods_table_diff(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should that be called create_component_handlers_table_diff to be consistent in our terminology?

Comment on lines +37 to +38
old_svc_methods: &[HandlerMetadata],
new_svc_methods: &[HandlerMetadata],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

old_component_handlers?

cli/src/ui/service_methods.rs Show resolved Hide resolved
crates/admin/src/rest_api/deployments.rs Show resolved Hide resolved
Comment on lines 26 to 30
# Encoding/Decoding
bytes = { workspace = true }
serde = { workspace = true }
serde_with = { workspace = true }
serde_json = { workspace = true }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I think we are no longer trying to group dependencies logically because it increases the maintenance burden and there is sometimes no clear grouping. What we try to do is to group restate dependencies and others together.

@slinkydeveloper
Copy link
Contributor Author

@tillrohrmann will address your comments in my next PR when I finish to get rid of all the proto stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants