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.

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

Singularity Request - Timezone field (API) #1150

Merged
merged 14 commits into from Aug 5, 2016
Merged

Conversation

@ghost
Copy link

ghost commented Jul 15, 2016

This PR extends the Singularity Request code to accept an optional (string) timezone through the API. Local testing passed.

Still needs frontend changes to make the timezones accessible to the end user.

/cc @ssalinas @Calvinp
/fixes #1002

@@ -24,6 +25,7 @@
private final Optional<String> schedule;
private final Optional<String> quartzSchedule;
private final Optional<ScheduleType> scheduleType;
private final Optional<TimeZone> scheduledTimeZone;

This comment has been minimized.

Copy link
@tpetr

tpetr Jul 15, 2016

Member

i'd keep the naming consistent and go with scheduleTimeZone

Optional<TimeZone> scheduledTimeZone = request.getScheduledTimeZone();
if (scheduledTimeZone.isPresent()) {
cronExpression.setTimeZone(scheduledTimeZone.get());
}

This comment has been minimized.

Copy link
@tpetr

tpetr Jul 15, 2016

Member

nitpick, but the variable here is probably unnecessary. also, if the variable is needed, try to make all your variables final unless they definitely need to be mutable (see above for an example)

@tpetr
Copy link
Member

tpetr commented Jul 15, 2016

Thanks for the PR! Could you add a unittest that exercises this feature? It would also be good to verify that setting a bogus timezone fails in an appropriate way (i.e. you get a meaningful error message back from the API)

MattCCS added 5 commits Jul 15, 2016
…kson from auto-validating); removed unnecessary memoization
…propriately to handle a string instead of a timezone
US/Eastern and GMT are daylight-savings-respecting timezone
codes, but EST is not, and may lead to confusion if used.
This error code tries to suggest better alternatives.
@ghost ghost self-assigned this Jul 18, 2016
MattCCS added 2 commits Jul 18, 2016
Having trouble getting a PENDING task after POSTing two scheduled tasks.
Fix will follow in time.
@ghost ghost removed their assignment Jul 19, 2016
MattCCS added 2 commits Jul 19, 2016
Two primary issues contributed to this test's delay:
- 2099 is the last valid year in Quartz 1, and
- getPendingTaskIds() returns in random order.

The latter was made clear immediately, but the former
was elusive;  getNextRunAt() simply returned a null pointer,
despite Quartz admitting that the cron expression was valid.
@@ -9,6 +9,9 @@
import java.util.Set;
import java.util.concurrent.TimeUnit;

import javax.ws.rs.WebApplicationException;

import org.apache.commons.jexl2.UnifiedJEXL.Exception;

This comment has been minimized.

Copy link
@ssalinas

ssalinas Jul 19, 2016

Member

don't think this import is used anywhere

This comment has been minimized.

Copy link
@MattCCS

MattCCS Jul 19, 2016

Contributor

The WebApplicationException is used, but yes I don't believe the JEXL one is.

.setScheduleTimeZone(Optional.of("GMT"))
.build();

requestResource.postRequest(req);

This comment has been minimized.

Copy link
@ssalinas

ssalinas Jul 19, 2016

Member

this test isn't validating anything separate and isn't asserting anything, I don't think we need it. Testing the post/validation of a valid timezone is already covered by the more detailed test you wrote further down

}

// GMT happens first, so EST is a larger timestamp
Assert.assertEquals(nextRunEST - nextRunGMT, fiveHoursInMilliseconds);

This comment has been minimized.

Copy link
@tpetr

tpetr Jul 19, 2016

Member

Consider using TimeUnit.HOURS.toMillis() instead

@@ -165,6 +166,12 @@ public SingularityRequest checkSingularityRequest(SingularityRequest request, Op
checkBadRequest(!request.getScheduleType().isPresent(), "ScheduleType can only be set for scheduled requests");
}

if (request.getScheduleTimeZone().isPresent()) {
if (!ArrayUtils.contains(TimeZone.getAvailableIDs(), request.getScheduleTimeZone().get())) {
badRequest("scheduleTimeZone %s was not a valid timezone code (e.g. 'US/Eastern' or 'GMT')", request.getScheduleTimeZone().get());

This comment has been minimized.

Copy link
@tpetr

tpetr Jul 19, 2016

Member

Is there an ISO or RFC standard we can reference so that users know what they have to choose from?

This comment has been minimized.

Copy link
@ssalinas

ssalinas Jul 19, 2016

Member

As far as I can tell, the getAvailableIds() list is maintained by Oracle, not necessarily conforming to any particular standard (that I can find).

https://garygregory.wordpress.com/2013/06/18/what-are-the-java-timezone-ids/

This comment has been minimized.

Copy link
@tpetr

tpetr Jul 19, 2016

Member

We can just mention that, then -- something like "... does not map to a valid Java TimeZone object." We should also make sure all the verb tenses match, i believe the rest are present tense

This comment has been minimized.

Copy link
@MattCCS

MattCCS Jul 19, 2016

Contributor

One other is not present tense, but I can make this change and align the tenses as well

MattCCS added 2 commits Jul 19, 2016
@MattCCS MattCCS added the hs_stable label Jul 22, 2016
@ssalinas ssalinas modified the milestone: 0.10.0 Jul 29, 2016
@ssalinas
Copy link
Member

ssalinas commented Aug 3, 2016

@Calvinp has this field been added to the UI in the rewrite? If not can we add it?

@Calvinp
Copy link
Contributor

Calvinp commented Aug 4, 2016

It has been, yes. See #1182

@ssalinas
Copy link
Member

ssalinas commented Aug 5, 2016

Going to merge this since UI field has been added in the rewrite

@ssalinas ssalinas merged commit c683d70 into master Aug 5, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@ssalinas ssalinas deleted the singularity_request_timezone branch Aug 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.