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.

Sign up for GitHub

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

feat: add optimus replay list #25

Merged
merged 7 commits into from
Sep 6, 2021
Merged

feat: add optimus replay list #25

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
55b0dc6
feat: add get replay list
arinda-arif Aug 20, 2021
5e972d9
fix: change get replay list url
arinda-arif Aug 20, 2021
2948d74
refactor: rearrange optimus replay path
arinda-arif Aug 24, 2021
728e88f
refactor: use proper http mapping on optimus replay custom method (dr…
arinda-arif Aug 24, 2021
5723ffe
refactor: change optimus GetReplayList to ListReplays to follow metho…
arinda-arif Aug 24, 2021
dbb519a
refactor: change optimus replay dry run method name to follow custom …
arinda-arif Aug 25, 2021
f5401d0
fix: solve optimus api conflict, change replayspec field type, remove…
arinda-arif Aug 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 26 additions & 4 deletions odpf/optimus/runtime_service.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,19 +179,24 @@ service RuntimeService {
}
rpc ReplayDryRun(ReplayRequest) returns (ReplayDryRunResponse) {
option (google.api.http) = {
post: "/v1/project/{project_name}/job/{job_name}/replay-dry-run"
post: "/v1/project/{project_name}/replay/{job_name}/dryrun"
Copy link
Member

@ravisuhag ravisuhag Aug 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

sinc dryrun is custom method. it can go as :dryrun or maybe just :dry ?
https://cloud.google.com/apis/design/custom_methods

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense. let me change it to :dryrun and test it

Copy link
Member

Choose a reason for hiding this comment

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

@ravisuhag using colon for identifying a custom verb is one way of doing it, other way is to split these custom verbs via slash. The advantage we have here when not using :verb is it avoid conflicts with the resource name. For example

/v1/project/{project_name}/replay/{job_name}/dryrun
/v1/project/{project_name}/replay/{job_name}:dryrun

Also I wonder how will the gateway regex be able to differentiate that when a URL looks like

/v1/project/test-project/replay/test-job:dryrun

job_name is test-job:dryrun or test-job? So these colon based verbs are nice when we have a fixed endpoint for example like

/v1/project/{project_name}/replay:dryrun

and have everything in the POST payload but then we won't be able to do GET calls nicely.

Its very rare we will be going to allow a job name that contains a / whereas a job name can contain : now or in the future. These colon based verbs can be used if we split them after a slash though like

/v1/project/{project_name}/replay/{job_name}/:dryrun

But then this looks like more of a choice of convention. For example github follows without colon custom verbs. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Ok cool. fine with slash as well.

body: "*"
};
}
rpc Replay(ReplayRequest) returns (ReplayResponse) {
option (google.api.http) = {
post: "/v1/project/{project_name}/job/{job_name}/replay"
post: "/v1/project/{project_name}/replay/{job_name}"
body: "*"
};
}
rpc GetReplayStatus(GetReplayStatusRequest) returns (GetReplayStatusResponse) {
option (google.api.http) = {
get: "/v1/project/{project_name}/job/{job_name}/replay/{id}"
get: "/v1/project/{project_name}/replay/{job_name}/{id}"
};
}
rpc GetReplayList(GetReplayListRequest) returns (GetReplayListResponse) {
Copy link
Member

Choose a reason for hiding this comment

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

What if we call ListReplays

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ReplayList sounds better to me, but you have concern on this? And should we change the GetReplayStatus too?

Copy link
Member

Choose a reason for hiding this comment

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

are we considering replay as a resource? In that case, I guess it should be replays in endpoint and ListReplays in the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

replays seems more descriptive. but i'm seeing other endpoints are not in a plural word, for example when getting projects or resource specs. thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

understood why ListReplays is better, just went through the method naming convention.

Copy link
Contributor Author

@arinda-arif arinda-arif Aug 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

seems Optimus is using singular in all the endpoint, do you think it is acceptable to keep as it is @ravisuhag ? if not, we need to apply the changes to the others as well to keep it in uniform. cc @kushsharma

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Planning to resolve this and thinking to just follow the current Optimus convention to use singular, and if a change in convention is needed, will handle it in a different issue. Please let me know if you have any concern on this, @ravisuhag

option (google.api.http) = {
get: "/v1/project/{project_name}/replay"
};
}
// TODO(kush.sharma): disabled ATM
Expand Down Expand Up @@ -660,4 +665,21 @@ message RegisterJobEventRequest {
JobEvent event = 4;
}

message RegisterJobEventResponse {}
message RegisterJobEventResponse {}

message GetReplayListRequest {
string project_name = 1;
}

message GetReplayListResponse {
repeated ReplaySpec replay_list = 1;
}

message ReplaySpec {
string id = 1;
string job_name = 2;
string start_date = 3;
Copy link
Member

Choose a reason for hiding this comment

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

why not google.protobuf.Timestamp

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 fine either way, but since start and end date is a date, i figured the response we are sending is already represented as expected, not in protobuf timestamp. any concern on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

discussed this and decided to use timestamp

string end_date = 4;
string state = 5;
google.protobuf.Timestamp created_at = 6;
}