Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Making Schedule interface Writeable #72

Merged
merged 3 commits into from
Sep 17, 2020
Merged

Making Schedule interface Writeable #72

merged 3 commits into from
Sep 17, 2020

Conversation

saratvemulapalli
Copy link
Contributor

@saratvemulapalli saratvemulapalli commented Sep 15, 2020 •
edited
Loading

Making Schedule interface Writeable and relevant changes for CronScheduler and IntervalScheduler classes.

  • This is needed for converting Anomaly Detection API's to integrate with RestActions (Which is needed for supporting FGAC for AD).

Issue
opendistro-for-elasticsearch/anomaly-detection#195

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@saratvemulapalli
Making Schedule interface Writeable and relevant changes for CronSche…
e62af43 
…duler and IntervalScheduler classes.

- This is needed for converting Anomaly Detection API's to integrate with RestActions.
@saratvemulapalli saratvemulapalli changed the title Making Schedule interface Writeable and relevant changes for CronSche… Making Schedule interface Writeable Sep 15, 2020
weicongs-amazon
weicongs-amazon reviewed Sep 15, 2020
View reviewed changes
Copy link
Contributor

@weicongs-amazon weicongs-amazon left a comment

Choose a reason for hiding this comment

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

I don't see concern for the code itself. btw, have we synced up with the team about why this change is necessary and what's the impact on other clients?

@saratvemulapalli
Copy link
Contributor Author

I don't see concern for the code itself. btw, have we synced up with the team about why this change is necessary and what's the impact on other clients?

Thanks @weicongs-amazon for taking a look. I did have a brief chat with @skkosuri-amzn and he agree's with the change (I'll wait for him to review the PR).
Also other clients which are using this is only ISM, since we are just extending the functionality I believe its ok.

dbbaughe
dbbaughe reviewed Sep 15, 2020
View reviewed changes
...n/java/com/amazon/opendistroforelasticsearch/jobscheduler/spi/schedule/IntervalSchedule.java Outdated
startTime = input.readInstant();
interval = input.readInt();
unit = input.readEnum(ChronoUnit.class);
intervalInMillis = input.readLong();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why write/read this instead of constructing based off interval and unit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question, this constructor is needed when the input is a stream. This stream is the TCP communication between nodes and helps us serialize and de-serialize information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had an offline chat with @dbbaughe. Sure makes sense. I'll make the change.

dbbaughe
dbbaughe previously approved these changes Sep 15, 2020
View reviewed changes
@saratvemulapalli saratvemulapalli added the refactor A change intended to improve the design or structure while preserving its functionality label Sep 15, 2020
@skkosuri-amzn
Copy link

Thanks @saratvemulapalli for the changes. Looks good. You plan to add UT to exercise writeTo and readFrom?

@saratvemulapalli
Copy link
Contributor Author

Thanks @saratvemulapalli for the changes. Looks good. You plan to add UT to exercise writeTo and readFrom?

Sure thanks for taking a look. I'll add some UTs.

@saratvemulapalli saratvemulapalli added enhancement An improvement on the existing feature’s functionalities and removed refactor A change intended to improve the design or structure while preserving its functionality labels Sep 17, 2020
@saratvemulapalli saratvemulapalli merged commit 28f0449 into master Sep 17, 2020
@saratvemulapalli saratvemulapalli deleted the writeable-changes branch September 17, 2020 21:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@weicongs-amazon weicongs-amazon weicongs-amazon left review comments

@dbbaughe dbbaughe dbbaughe approved these changes

@skkosuri-amzn skkosuri-amzn skkosuri-amzn approved these changes

Assignees
No one assigned
Labels
enhancement An improvement on the existing feature’s functionalities
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@saratvemulapalli @skkosuri-amzn @dbbaughe @weicongs-amazon