Skip to content

feat: add optimus replay list #25

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

Merged
arinda-arif merged 7 commits into from
Sep 6, 2021
Merged

feat: add optimus replay list #25

Changes from 2 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
24 changes: 23 additions & 1 deletion odpf/optimus/runtime_service.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,11 @@ service RuntimeService {
get: "/v1/project/{project_name}/job/{job_name}/replay/{id}"
};
}
rpc GetReplayList(GetReplayListRequest) returns (GetReplayListResponse) {
Copy link
Member

@kushsharma kushsharma Aug 23, 2021

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

@arinda-arif arinda-arif Aug 23, 2021

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

@ravisuhag ravisuhag Aug 24, 2021

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

@arinda-arif arinda-arif Aug 24, 2021

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

@arinda-arif arinda-arif Aug 24, 2021

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

@arinda-arif arinda-arif Sep 2, 2021

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/list"
Copy link
Member

@kushsharma kushsharma Aug 23, 2021

Choose a reason for hiding this comment

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

we should make it follow REST principles, do we need list?

Copy link
Contributor Author

@arinda-arif arinda-arif Aug 24, 2021

Choose a reason for hiding this comment

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

updated to not use list

};
}
// TODO(kush.sharma): disabled ATM
//rpc DeleteResource(DeleteResourceRequest) returns (DeleteResourceResponse) {}

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

@kushsharma kushsharma Aug 23, 2021

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

@arinda-arif arinda-arif Aug 23, 2021

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

@arinda-arif arinda-arif Sep 2, 2021

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;
}