Starred requests persistence #1219

Merged
merged 55 commits into from Nov 22, 2016

Conversation

Projects
None yet
5 participants
@MattCCS
Contributor

MattCCS commented Aug 15, 2016

This PR enables "starred" Singularity requests to persist in Zookeeper, rather than be stored client-side, so clearing the cache or changing devices won't require re-starring.

/cc @ssalinas @tpetr

@Calvinp

This comment has been minimized.

Show comment
Hide comment
@Calvinp

Calvinp Aug 15, 2016

Contributor

Are we still planning on having endpoints to add or remove one star at a time?

Contributor

Calvinp commented Aug 15, 2016

Are we still planning on having endpoints to add or remove one star at a time?

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Aug 15, 2016

Member

@MattCCS as part of the implementation, I believe we also discussed adding the SingularityUserSettings into the SingularityUserHolder object so we can grab things on the /auth/user call when possible. Can we add that in?

Member

ssalinas commented Aug 15, 2016

@MattCCS as part of the implementation, I believe we also discussed adding the SingularityUserSettings into the SingularityUserHolder object so we can grab things on the /auth/user call when possible. Can we add that in?

+ public void setUserSettings(
+ @ApiParam("The user id to use") @QueryParam("userId") String userId,
+ @ApiParam("The new settings") SingularityUserSettings settings) {
+ userManager.updateUserSettings(SingularityValidator.encodeZkName(userId), settings);

This comment has been minimized.

@tpetr

tpetr Aug 15, 2016

Member

this ZK encoding stuff should be inside UserManager -- the point of it is to encapsulate away all ZK-related stuff

@tpetr

tpetr Aug 15, 2016

Member

this ZK encoding stuff should be inside UserManager -- the point of it is to encapsulate away all ZK-related stuff

This comment has been minimized.

@MattCCS

MattCCS Aug 15, 2016

Contributor

Will fix.

@MattCCS

MattCCS Aug 15, 2016

Contributor

Will fix.

@@ -436,4 +437,12 @@ private boolean isValidInteger(String strValue) {
return false;
}
}
+
+ public static String encodeZkName(String name) {
+ checkBadRequest(name != null, "Name may not be a null value");

This comment has been minimized.

@ssalinas

ssalinas Aug 15, 2016

Member

Can use Strings.isNullOrEmpty(name) to hit both these lines in one go

@ssalinas

ssalinas Aug 15, 2016

Member

Can use Strings.isNullOrEmpty(name) to hit both these lines in one go

This comment has been minimized.

@MattCCS

MattCCS Aug 15, 2016

Contributor

Oh, handy, thanks.

@MattCCS

MattCCS Aug 15, 2016

Contributor

Oh, handy, thanks.

@@ -436,4 +437,12 @@ private boolean isValidInteger(String strValue) {
return false;
}
}
+
+ public static String encodeZkName(String name) {

This comment has been minimized.

@ssalinas

ssalinas Aug 15, 2016

Member

All our other method names are named in the format check..., maybe checkUserId or checkAndEncodeUserId to show what's being returned + be consistent with other methods in the class

@ssalinas

ssalinas Aug 15, 2016

Member

All our other method names are named in the format check..., maybe checkUserId or checkAndEncodeUserId to show what's being returned + be consistent with other methods in the class

This comment has been minimized.

@MattCCS

MattCCS Aug 15, 2016

Contributor

Right, my bad, will fix.

@MattCCS

MattCCS Aug 15, 2016

Contributor

Right, my bad, will fix.

+ this.settingsTranscoder = settingsTranscoder;
+ }
+
+ private String getUserSettingsPath(String id) {

This comment has been minimized.

@ssalinas

ssalinas Aug 15, 2016

Member

nit, but would be good to use userId (or something else more descriptive than id) as the variable name. There are many things with ids that get passed around Singularity

@ssalinas

ssalinas Aug 15, 2016

Member

nit, but would be good to use userId (or something else more descriptive than id) as the variable name. There are many things with ids that get passed around Singularity

This comment has been minimized.

@MattCCS

MattCCS Aug 15, 2016

Contributor

Right, will fix.

@MattCCS

MattCCS Aug 15, 2016

Contributor

Right, will fix.

@MattCCS

This comment has been minimized.

Show comment
Hide comment
@MattCCS

MattCCS Aug 15, 2016

Contributor

@Calvinp I think we're instead going to do the unioning client-side, and post the entire union back to ZK. This is done server-side now.

Contributor

MattCCS commented Aug 15, 2016

@Calvinp I think we're instead going to do the unioning client-side, and post the entire union back to ZK. This is done server-side now.

@ssalinas ssalinas changed the title from Starred requests persistence to (WIP) Starred requests persistence Aug 15, 2016

@MattCCS

This comment has been minimized.

Show comment
Hide comment
@MattCCS

MattCCS Aug 18, 2016

Contributor

Update: I merged the master branch back into this PR after decaf was merged into master (i.e. tested and working post-decaf).

Contributor

MattCCS commented Aug 18, 2016

Update: I merged the master branch back into this PR after decaf was merged into master (i.e. tested and working post-decaf).

@Calvinp Calvinp referenced this pull request Aug 18, 2016

Closed

Starred requests persistence frontend #1229

7 of 8 tasks complete

@MattCCS MattCCS added the hs_staging label Aug 18, 2016

@ssalinas ssalinas modified the milestone: 0.11.0 Aug 19, 2016

MattCCS added some commits Aug 23, 2016

+ public void deleteStarredRequestIds(String userId, Set<String> starredRequestIds) {
+ final String path = getUserSettingsPath(userId);
+ final Optional<SingularityUserSettings> settings = getData(path, settingsTranscoder);
+ if (!settings.isPresent()) {

This comment has been minimized.

@tpetr

tpetr Sep 2, 2016

Member

do we even need to save if user settings aren't present?

@tpetr

tpetr Sep 2, 2016

Member

do we even need to save if user settings aren't present?

This comment has been minimized.

@MattCCS

MattCCS Sep 2, 2016

Contributor

I don't believe so, no! Will fix

@MattCCS

MattCCS Sep 2, 2016

Contributor

I don't believe so, no! Will fix

+ private String getAuthUserId() {
+ checkBadRequest(user.isPresent() && user.get().getName().isPresent(), "Singularity userId must be provided by auth");
+ return user.get().getName().get();
+ }

This comment has been minimized.

@tpetr

tpetr Sep 2, 2016

Member

This method needs some work:

  • The error message isn't accurate because we're not using the userId query param anymore.
  • Being unauthenticated should return an Unauthorized response, not a Bad Request response. Use SingularityAuthorizationHelper as a guide here; it uses checkUnauthorized(user.isPresent(), "Please log in to perform this action."); for stuff like this.
  • It's using the name field of SingularityUser (Thomas Petr) when it should be using the id field (tpetr).
@tpetr

tpetr Sep 2, 2016

Member

This method needs some work:

  • The error message isn't accurate because we're not using the userId query param anymore.
  • Being unauthenticated should return an Unauthorized response, not a Bad Request response. Use SingularityAuthorizationHelper as a guide here; it uses checkUnauthorized(user.isPresent(), "Please log in to perform this action."); for stuff like this.
  • It's using the name field of SingularityUser (Thomas Petr) when it should be using the id field (tpetr).

This comment has been minimized.

@MattCCS

MattCCS Sep 2, 2016

Contributor

Ah, my bad. So, should I be checking authEnabled before I do the checkAuthorized() call (as happens in SingularityAuthorizationHelper)? Or should I just be calling checkAuthorized() every time?

@MattCCS

MattCCS Sep 2, 2016

Contributor

Ah, my bad. So, should I be checking authEnabled before I do the checkAuthorized() call (as happens in SingularityAuthorizationHelper)? Or should I just be calling checkAuthorized() every time?

@ssalinas ssalinas modified the milestones: 0.11.0, 0.12.0 Sep 14, 2016

@ssalinas ssalinas added the hs_qa label Oct 31, 2016

@@ -2,6 +2,8 @@
import static com.google.common.base.Preconditions.checkNotNull;
+import javax.ws.rs.HEAD;

This comment has been minimized.

@tpetr

tpetr Oct 31, 2016

Member

i think this came in from a merge conflict

@tpetr

tpetr Oct 31, 2016

Member

i think this came in from a merge conflict

@ssalinas ssalinas modified the milestones: 0.12.0, 0.13.0 Nov 4, 2016

@ssalinas ssalinas added the hs_stable label Nov 15, 2016

@ssalinas

This comment has been minimized.

Show comment
Hide comment
@ssalinas

ssalinas Nov 22, 2016

Member

@tpetr any problem with merging this one now?

Member

ssalinas commented Nov 22, 2016

@tpetr any problem with merging this one now?

@tpetr tpetr merged commit 20f0b94 into master Nov 22, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@ssalinas ssalinas deleted the starred_requests_persistence branch Feb 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment