Allow editing of existing repair schedules #1119

jdonenine · 2021-09-03T04:08:33Z

Adds support within the UI for the editing of existing repair schedules. Allows for the editing of a select set of fields as discussed in #1006 :

days_between (which will force recomputing next_activation)
intensity
owner
repair_parallelism
segment_count_per_node

The editing is provided for via a new component that is decoupled from the schedules-list component so that it can potentially be re-used in other places:

The form itself includes all of the same fields that are present in the "Add Schedule" form, but most of them are presented in read-only fashion when used for editing (this is where the form component could later be enhanced to allow it to support edit and add workflows -- it doesn't do that now).

Edits are saved via a new API endpoint: PATCH /repair_schedule/ that is provided here.

Fixes #1006

* Add new component to serve as a more flexible and basic repair editing form * Integration that form into an edit dialog within the schedule list

* Adds a new method to allow updating of a given repair schedule resource * Adds some unit testing of functionality used within the new endpoint

* Enable CORS for PATCH requests * API call from within the UI

jdonenine · 2021-09-03T04:11:30Z

src/server/src/main/java/io/cassandrareaper/resources/RepairScheduleResource.java

+  public Response patchRepairSchedule(
+          @Context UriInfo uriInfo,
+          @PathParam("id") UUID repairScheduleId,
+          @QueryParam("owner") Optional<String> owner,


@adejanovski I don't love passing these as query parameters and not as a resource in the body, but this is what was done for the PUT request that is currently used to update status, so I followed that model - was what was done for the other requests preferred or would you want to break from the convention?

If the REST convention is to pass them in the body then I'm happy for you to do it, even if it doesn't follow what we've done elsewhere in the code (as any other OSS project, it's made of many peeps knowledge or lack of, so it's not necessarily done the right way).

Cool, I went ahead and made the change. It has the other added benefit of simplifying the validation pieces and lets Dropwizard handle it by itself with the addition of a few custom validators.

it simplifies the resource code and you also get some nicer error responses from Dropwizard that can be useful, like:

PATCH http://127.0.0.1:8080/repair_schedule/da706f30-0c3e-11ec-be46-915dd6e16900 { "owner": "", "intensity": 0.5, "repair_parallelism": "DATACENTER_AWARE", "scheduled_days_between": 60, "segment_count_per_node": -10 }

422 { "errors": [ "segmentCountPerNode must be greater than or equal to 1", "daysBetween must be less than or equal to 31", "owner must not be empty if it is provided" ] }

Dropwizard provides that response automatically based on the validation rules configured for the class.

src/ui/app/jsx/repair-schedule-form.jsx

src/server/src/main/java/io/cassandrareaper/resources/RepairScheduleResource.java

jdonenine · 2021-09-03T20:53:44Z

src/server/src/main/java/io/cassandrareaper/resources/RepairScheduleResource.java

@@ -52,6 +51,7 @@

 import com.google.common.base.Preconditions;
 import com.google.common.collect.Lists;
+import io.dropwizard.jersey.PATCH;


@adejanovski to make Checkstyle happy I had to go about changing a bunch of imports that IntelliJ had automatically organized, which I found a bit silly :-)

I've fixed things here in this PR, so we don't have to worry about it at the moment, but what would you think about removing this Checkstyle rule?

<property name="customImportOrderRules" value="SAME_PACKAGE(2)###STANDARD_JAVA_PACKAGE###THIRD_PARTY_PACKAGE###STATIC"/>

That enforces import ordering -- my feeling is that we should let the IDEs do their job and organize for us. Or perhaps think about moving to something like https://github.com/google/google-java-format that can actively enforce formatting, would probably be nice to do similar in the UI code with prettier as well. What do you think?

I understand your frustration here but I'm reluctant to change the formatting rules to rely on IDEs instead. They will most probably apply different orderings which will create some noise in PRs as there's gonna be some unnecessary line changes.

Instead, we could maybe make it so that the rule matches IntelliJ's ordering of imports:

To configure the check so that it matches default IntelliJ IDEA formatter configuration (tested on v14): group of static imports is on the bottom groups of non-static imports: all imports except of "javax" and "java", then "javax" and "java" imports will be sorted in the groups groups are separated by one blank line Note: "separated" option is disabled because IDEA default has blank line between "java" and static imports, and no blank line between "javax" and "java" <module name="CustomImportOrder"> <property name="customImportOrderRules" value="THIRD_PARTY_PACKAGE###SPECIAL_IMPORTS###STANDARD_JAVA_PACKAGE###STATIC"/> <property name="specialImportsRegExp" value="^javax\."/> <property name="standardPackageRegExp" value="^java\."/> <property name="sortImportsInGroupAlphabetically" value="true"/> <property name="separateLineBetweenGroups" value="false"/> </module>

this was pasted from the checkstyle docs: https://checkstyle.org/config_imports.html#ImportOrder_Examples

Apparently there's some workaround for the java/javax imports which can't be dealt with appropriately by IntelliJ: https://stackoverflow.com/a/64243861/5106246

* Changes the provided PATCH endpoint to require body content, not query params * That allows for more flexible validation to be handled directly by Dropwizard * Adds Lombok dependency

codecov · 2021-09-04T04:22:22Z

Codecov Report

Merging #1119 (b107474) into master (e64b689) will decrease coverage by 3.91%.
The diff coverage is 92.64%.

@@            Coverage Diff             @@
##           master    #1119      +/-   ##
==========================================
- Coverage   74.15%   70.23%   -3.92%     
==========================================
  Files         133      136       +3     
  Lines        9786     9852      +66     
  Branches     1018     1030      +12     
==========================================
- Hits         7257     6920     -337     
- Misses       1928     2330     +402     
- Partials      601      602       +1

Impacted Files	Coverage Δ
...ain/java/io/cassandrareaper/ReaperApplication.java	`70.08% <0.00%> (-5.99%)`	⬇️
...n/java/io/cassandrareaper/core/RepairSchedule.java	`93.33% <ø> (-0.29%)`	⬇️
...er/validators/EditableRepairScheduleValidator.java	`57.14% <57.14%> (ø)`
...ndrareaper/validators/NullOrNotBlankValidator.java	`66.66% <66.66%> (ø)`
...o/cassandrareaper/core/EditableRepairSchedule.java	`100.00% <100.00%> (ø)`
...sandrareaper/resources/RepairScheduleResource.java	`59.00% <100.00%> (+6.34%)`	⬆️
...n/java/io/cassandrareaper/jmx/CompactionProxy.java	`37.83% <0.00%> (-35.14%)`	⬇️
.../main/java/io/cassandrareaper/ReaperException.java	`33.33% <0.00%> (-33.34%)`	⬇️
...io/cassandrareaper/jmx/HostConnectionCounters.java	`50.00% <0.00%> (-29.42%)`	⬇️
...a/io/cassandrareaper/storage/CassandraStorage.java	`69.42% <0.00%> (-15.16%)`	⬇️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e64b689...b107474. Read the comment docs.

* Adds additional testing around the validators added and the new PATCH resource itself * Modifies the UI code to pass request to the new resource via the body instead of query parameters

* Adds a small test around the changes to the structure of the RepairSchedule and EditableRepairSchedule classes

* For manually thrown errors respond with an entity in the same way DropWizard's validation would for consistency of the API responses * Add additional testing around those error conditions

adejanovski

That's an impressively crafted PR @jdonenine 👏
Lots of testing and added safety through the use of validators are very much appreciated.

I'm concerned that adding Lombok to the project requires a wider conversation with other contributors and I think we should keep it out of the scope of this PR.
I mislead you with setting the lower bound to 1 for the segment count per node. 0 is very much valid and will translate into the default number of segments (which should be 64 in the default configuration).

src/server/src/main/java/io/cassandrareaper/core/EditableRepairSchedule.java

adejanovski · 2021-09-13T08:07:03Z

src/server/src/main/java/io/cassandrareaper/core/EditableRepairSchedule.java

+  protected Integer daysBetween;
+
+  @JsonProperty(value = "segment_count_per_node")
+  @Min(value = 1)


I was wrong in suggesting 1 as minimum value: if set to 0 it currently translates to the default number of segments which is applied when the repair run is created.
Currently it prevents from editing a schedule that had 0 segment per node if the value isn't set explicitly (which is then not consistent with the initial schedule creation).
You can remove the minimum bound to allow 0 as valid.
My bad for this.

No worries at all, I updated the min to be 0 in the API and the UI.

cool, thanks!

src/server/src/main/java/io/cassandrareaper/resources/RepairScheduleResource.java

adejanovski · 2021-09-13T08:25:36Z

src/ui/app/jsx/repair-schedule-form.jsx

+    const segmentsPerNodeInvalid = !state.segments ||
+      state.segments == undefined ||
+      state.segments == null ||
+      state.segments < 1 ||


We can relax this constraint as mentioned in my previous comment and allow 0 as segment count per node.

* Remove dependency * Add necessary constructor/getters/setters as necessary to replace it

* Switching to a minimum of 0 (which implies the default) per review comments

adejanovski

Aside from one additional change to make it so that the Save button is enabled by default in the edit schedule modal, it looks good to me 👍

adejanovski · 2021-09-14T07:36:57Z

src/ui/app/jsx/repair-schedule-form.jsx

@@ -110,7 +110,7 @@ const RepairScheduleForm = CreateReactClass({
    const segmentsPerNodeInvalid = !state.segments ||
      state.segments == undefined ||
      state.segments == null ||
-      state.segments < 1 ||
+      state.segments < 0 ||


The segment count per node is considered invalid by default because it's initialized as '' instead of 0 here: ec55807#diff-166c0877429b0e6dad5328286ab5af08f46ff06c0ee1a048483f52ffa5dd9d3eR43

This results in the Save button being disabled unless the value is changed.
Could you make it so that 0 is the default value and the Save button is enabled when editing without making a change?

It's definitely true that the Save button is disabled until a value is changed, but not because of the validity of the value. The validation check is only triggered on changes to the form, so that you can't save an non-dirty form so to speak. It will only let you save a form that has been changed -- although it's not a deep inspection that compares each value over and over again, just for the record.

But, that aside, I do notice that I have the validation wrong, I shouldn't be checking !state.segments, because it's a number and that could screw up, I will fix that now.

I also failed to check the intensity value there, I've added that as well in the new commit.

* Don't check for !(numeric value) * Add check for intensity

adejanovski

It looks great now @jdonenine and works as expected!
Could you please merge the PR using the "Squash and merge" button and rework the commit message in the GitHub UI so that it nicely describes the main changes?
Thanks!

jdonenine · 2021-09-14T13:49:07Z

Merged, thanks for all the help and tips @adejanovski

jdonenine added 3 commits August 31, 2021 22:58

Initial UI enhancements

073c4ad

* Add new component to serve as a more flexible and basic repair editing form * Integration that form into an edit dialog within the schedule list

Add PATCH /repair-schedule endpoint

1ac60b0

* Adds a new method to allow updating of a given repair schedule resource * Adds some unit testing of functionality used within the new endpoint

Connect new UI to new API

a007798

* Enable CORS for PATCH requests * API call from within the UI

jdonenine added the ui label Sep 3, 2021

jdonenine requested a review from adejanovski September 3, 2021 04:08

jdonenine commented Sep 3, 2021

View reviewed changes

src/ui/app/jsx/repair-schedule-form.jsx Outdated Show resolved Hide resolved

jdonenine commented Sep 3, 2021

View reviewed changes

src/server/src/main/java/io/cassandrareaper/resources/RepairScheduleResource.java Outdated Show resolved Hide resolved

jdonenine added 2 commits September 3, 2021 16:39

Address some checkstyle issues

e782887

More checkstyle stuff with the import ordering

8697328

jdonenine commented Sep 3, 2021

View reviewed changes

Shift validation directly to Dropwizard

1023e80

* Changes the provided PATCH endpoint to require body content, not query params * That allows for more flexible validation to be handled directly by Dropwizard * Adds Lombok dependency

jdonenine added 8 commits September 8, 2021 15:18

Additional tests and UI connect

c8a7409

* Adds additional testing around the validators added and the new PATCH resource itself * Modifies the UI code to pass request to the new resource via the body instead of query parameters

Additional testing

9db6be2

* Adds a small test around the changes to the structure of the RepairSchedule and EditableRepairSchedule classes

Remove unused constructor

356f5ef

Update error responses and testing

2faf5ce

* For manually thrown errors respond with an entity in the same way DropWizard's validation would for consistency of the API responses * Add additional testing around those error conditions

Test cleanup

a9eeedf

Test fix

4685417

Add not found test and cleanup

4bf715f

Testing additions

242ad51

jdonenine marked this pull request as ready for review September 13, 2021 03:01

adejanovski requested changes Sep 13, 2021

View reviewed changes

jdonenine added 2 commits September 13, 2021 07:56

Remove Lombok dependency

1142e15

* Remove dependency * Add necessary constructor/getters/setters as necessary to replace it

Change minimum segment count per node value

ec55807

* Switching to a minimum of 0 (which implies the default) per review comments

adejanovski requested changes Sep 14, 2021

View reviewed changes

Correct form validation logic

b107474

* Don't check for !(numeric value) * Add check for intensity

adejanovski approved these changes Sep 14, 2021

View reviewed changes

jdonenine merged commit 36e4f82 into thelastpickle:master Sep 14, 2021

jdonenine deleted the 1006--ui-edit-schedules branch September 14, 2021 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow editing of existing repair schedules #1119

Allow editing of existing repair schedules #1119

jdonenine commented Sep 3, 2021

jdonenine Sep 3, 2021

adejanovski Sep 3, 2021

jdonenine Sep 4, 2021

jdonenine Sep 3, 2021

adejanovski Sep 6, 2021

codecov bot commented Sep 4, 2021 •

edited

Loading

adejanovski left a comment

adejanovski Sep 13, 2021

jdonenine Sep 13, 2021

adejanovski Sep 14, 2021

adejanovski Sep 13, 2021

adejanovski left a comment •

edited

Loading

adejanovski Sep 14, 2021

jdonenine Sep 14, 2021

jdonenine Sep 14, 2021

adejanovski left a comment

jdonenine commented Sep 14, 2021

Allow editing of existing repair schedules #1119

Allow editing of existing repair schedules #1119

Conversation

jdonenine commented Sep 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 4, 2021 • edited Loading

Codecov Report

adejanovski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adejanovski left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adejanovski left a comment

Choose a reason for hiding this comment

jdonenine commented Sep 14, 2021

codecov bot commented Sep 4, 2021 •

edited

Loading

adejanovski left a comment •

edited

Loading