Resources #23

bergman · 2016-12-08T17:14:03Z

Missing:

tests

rouzwawi · 2016-12-08T18:24:18Z

styx-api-service/src/main/java/com/spotify/styx/api/CliResource.java

+            rc -> proxy("/" + arg("endpoint", rc) + "/" + arg("id", rc), rc)),
+        Route.async(
+            "PATCH", BASE + "/<endpoint>/<id>",
+            rc -> proxy("/" + arg("endpoint", rc) + "/" + arg("id", rc), rc))


You might want to replace these all with just a /<endpoint:path> parameter that will match the rest of the uri. see https://github.com/spotify/apollo/blob/master/apollo-api/docs/routing-engine.md#path-parameters

Thanks, was thinking of that but didn't know how to do it :)

rouzwawi · 2016-12-08T18:26:04Z

styx-api-service/src/main/java/com/spotify/styx/api/ResourceResource.java

+      resource = OBJECT_MAPPER.readValue(payload.get().toByteArray(), Resource.class);
+    } catch (IOException e) {
+      return Response.forStatus(Status.BAD_REQUEST.withReasonPhrase("Invalid payload."));
+    }


Use the EntityMiddleware to avoid having to do manual parsing and error handling.

rouzwawi · 2016-12-08T18:28:10Z

styx-common/src/main/java/com/spotify/styx/model/DataEndpoint.java

  @JsonCreator
  public static DataEndpoint create(
      @JsonProperty("id") String id,
      @JsonProperty("partitioning") Partitioning partitioning,
      @JsonProperty("docker_image") Optional<String> dockerImage,
      @JsonProperty("docker_args") Optional<List<String>> dockerArgs,
-      @JsonProperty("secret") Optional<Secret> secret) {
+      @JsonProperty("secret") Optional<Secret> secret,
+      @JsonProperty("resources") Optional<List<String>> resources) {


I think jackson can handle parsing a missing key directly into an empty list, without having to wrap it in an optional. For the resources field, a missing key and an empty list should have the same meaning.

rouzwawi · 2016-12-08T18:28:53Z

styx-common/src/main/java/com/spotify/styx/model/Resource.java

+  public abstract String id();
+
+  @JsonProperty
+  public abstract long concurrency();


Do we want to call it concurrency or limit? "Resource limit" sounds more natural to me than a "Resource concurrency".

The word limit might not convey that it gets replenished after use. Maybe we can look into what wording thread pools use?

rouzwawi · 2016-12-08T18:29:32Z

styx-api-service/src/main/java/com/spotify/styx/api/ResourceResource.java

+            json(), "DELETE", BASE + "/<rid>",
+            rc -> deleteResource(arg("rid", rc))),
+        Route.with(
+            json(), "PATCH", BASE + "/<rid>",


Shouldn't this be PUT?

rouzwawi · 2016-12-08T18:32:59Z

styx-scheduler-service/src/main/java/com/spotify/styx/StyxScheduler.java

@@ -451,7 +451,8 @@ private TriggerListener trigger(

    return (workflow, triggerId, instant) -> {
      try {
-        if (!storage.enabled(workflow.id()) || !storage.globalEnabled()) {
+        final Set<WorkflowId> enabledWorkflows = storage.enabledWorkflows();


Unrelated change? Why get all the enabled workflows instead of just checking for a specific one?

If it was changed because enabled(WorkflowId) is deprecated, then use storage.workflowState instead.

rouzwawi · 2016-12-08T18:35:14Z

styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java

+
+  Resource getResource(String id) {
+    return entityToResource(
+        datastore.get(datastore.newKeyFactory().kind(KIND_RESOURCE).newKey(id)));


Potential NPE if the lookup does not find a result.

rouzwawi · 2016-12-08T18:35:24Z

styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java

+  }
+
+  void postResource(Resource resource) {
+    datastore.put(resourceToEntity(resource));


Wrap in retries?

rouzwawi · 2016-12-08T18:37:15Z

styx-common/src/main/java/com/spotify/styx/storage/InMemStorage.java

@@ -20,9 +20,11 @@

 package com.spotify.styx.storage;

+import com.google.bigtable.repackaged.com.google.common.collect.ImmutableList;


Shaded import

rouzwawi · 2016-12-08T18:40:34Z

styx-common/src/main/java/com/spotify/styx/storage/Storage.java

@@ -116,14 +117,14 @@
   * @return true if the queried workflow is enabled
   */
  @Deprecated
-  boolean enabled(WorkflowId workflowId) throws IOException;
+  boolean isWorkflowEnabled(WorkflowId workflowId) throws IOException;


These were named with the argument type in mind, like store(Workflow), delete(WorkflowId), enabled(WorkflowId).

The names are all repetitive now with storeWorkflow(Workflow).

I can see that the last one is be better as isEnabled(WorkflowId), though.

If we want to rename them, then maybe the metric names in MeteredStorage should be aligned too.

Ah, forgot about the strings in MeteredStorage. Will have another pass at this.

Will keep enabledWorkflows and isEnabled but revert the others

Will revert all names :)

rouzwawi · 2016-12-09T16:26:42Z

styx-common/src/main/java/com/spotify/styx/storage/DatastoreStorage.java

+    return Optional.of(entityToResource(entity));
+  }
+
+  void postResource(Resource resource) throws IOException {


No transaction needed when only doing one operation

Fixed, thanks!

rouzwawi · 2016-12-13T09:12:05Z

@bergman should we wait for cli and tests or are you doing them as separate prs?

bergman · 2016-12-13T09:36:45Z

Let's do CLI and tests in a separate PR if you need this now. Just know that this code has never been actually run against datastore (mocked or otherwise).

rouzwawi · 2016-12-13T09:37:46Z

Then we should at least do tests in this PR.

rouzwawi · 2016-12-14T16:54:44Z

styx-api-service/src/test/java/com/spotify/styx/api/ResourceResourceTest.java

+    return Api.Version.V1.prefix() + ResourceResource.BASE + path;
+  }
+
+  private Connection setupBigTableMockTable() {


Do we have to set up mocked behaviour for bigtable for this test? I think it would be enough just to send in a plain mock, since none of the paths will interact with bigtable.

you're right, will mock instead

rouzwawi · 2016-12-14T16:56:12Z

styx-api-service/src/test/java/com/spotify/styx/api/ResourceResourceTest.java

+        awaitResponse(serviceHelper.request("GET", path("")));
+
+    assertThat(response, hasStatus(belongsToFamily(StatusType.Family.SUCCESSFUL)));
+    System.out.println("response.payload().get().utf8() = " + response.payload().get().utf8());


rouzwawi · 2016-12-14T16:59:12Z

styx-api-service/src/test/java/com/spotify/styx/api/ResourceResourceTest.java

+        awaitResponse(serviceHelper.request("PUT", path("/resource2"),
+            ByteString.encodeUtf8("{\"id\": \"resource2\", \"concurrency\": 3}")));
+    assertThat(putResponse, hasStatus(belongsToFamily(StatusType.Family.SUCCESSFUL)));
+    assertJson(putResponse, "id", equalTo("resource2"));


Try not to assert things that are not essential for the test. Checking the put response object is already done in the shouldUpdateResource test case.

yup, same for first listing i guess, it's the same case as shouldListMultipleResources except that one doesn't POST but uses datastore directly

hmm, but it makes sense there actually, will keep it

Yeah I was thinking about that but then thought that it makes sense to test that the changes are actually reflected in the api. But checking the put response is not directly related.

rouzwawi · 2016-12-14T17:08:13Z

LGTM 👍

bergman force-pushed the resource branch from de96d2b to 1658361 Compare December 8, 2016 17:15

rouzwawi suggested changes Dec 8, 2016

View reviewed changes

bergman force-pushed the resource branch 2 times, most recently from d688cb8 to 0992ccf Compare December 9, 2016 16:22

rouzwawi reviewed Dec 9, 2016

View reviewed changes

bergman force-pushed the resource branch 6 times, most recently from 1322c76 to b5df9e3 Compare December 9, 2016 17:11

rouzwawi approved these changes Dec 13, 2016

View reviewed changes

rouzwawi closed this Dec 13, 2016

rouzwawi reopened this Dec 13, 2016

bergman force-pushed the resource branch from b5df9e3 to 230ab4d Compare December 14, 2016 10:19

rouzwawi mentioned this pull request Dec 14, 2016

Limit Concurrency #29

Merged

2 tasks

rouzwawi reviewed Dec 14, 2016

View reviewed changes

resources

25221a8

bergman force-pushed the resource branch from 5f98bf1 to 25221a8 Compare December 14, 2016 17:09

bergman changed the title ~~WIP resource~~ Resources Dec 14, 2016

bergman merged commit f0e1f70 into master Dec 14, 2016

bergman deleted the resource branch December 14, 2016 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resources #23

Resources #23

bergman commented Dec 8, 2016 •

edited by rouzwawi

Loading

rouzwawi Dec 8, 2016

bergman Dec 9, 2016

rouzwawi Dec 8, 2016

rouzwawi Dec 8, 2016

rouzwawi Dec 8, 2016

bergman Dec 9, 2016

rouzwawi Dec 8, 2016

rouzwawi Dec 8, 2016

rouzwawi Dec 8, 2016

bergman Dec 9, 2016

rouzwawi Dec 8, 2016

bergman Dec 9, 2016

rouzwawi Dec 8, 2016

bergman Dec 9, 2016

rouzwawi Dec 8, 2016

bergman Dec 9, 2016

rouzwawi Dec 8, 2016

bergman Dec 9, 2016

bergman Dec 9, 2016

bergman Dec 9, 2016

rouzwawi Dec 9, 2016

bergman Dec 9, 2016

rouzwawi commented Dec 13, 2016

bergman commented Dec 13, 2016

rouzwawi commented Dec 13, 2016

rouzwawi Dec 14, 2016

bergman Dec 14, 2016

rouzwawi Dec 14, 2016

bergman Dec 14, 2016

rouzwawi Dec 14, 2016

bergman Dec 14, 2016

bergman Dec 14, 2016

rouzwawi Dec 14, 2016

rouzwawi commented Dec 14, 2016

		@@ -20,9 +20,11 @@

		package com.spotify.styx.storage;

		import com.google.bigtable.repackaged.com.google.common.collect.ImmutableList;

Resources #23

Resources #23

Conversation

bergman commented Dec 8, 2016 • edited by rouzwawi 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouzwawi commented Dec 13, 2016

bergman commented Dec 13, 2016

rouzwawi commented Dec 13, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rouzwawi commented Dec 14, 2016

bergman commented Dec 8, 2016 •

edited by rouzwawi

Loading