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

Add cluster namespace support #119

Merged
merged 5 commits into from Aug 15, 2017

Conversation

shuzhang1989
Copy link
Member

@shuzhang1989 shuzhang1989 commented Aug 9, 2017

It is a full stack change, motivation is to divide different clusters into namespace and cluster name. so realpin, rocksdb and scorpion will be 3 namespaces, and cluster name will be the next level in zk path.

Change list:

  1. Changed DB schema to add (namespace, name) as the composite primary key of tag table, and (namespace, name) as the composite foreign key of task table.
  2. Change all reference to String cluster to Cluster class, which contains namespace and name. Change all API interfaces to refer to Cluster class as input parameter.
  3. Changed all HTTP apis which needs to specify cluster to 2 levels: namespace/name.
  4. TaskQueue interfaces are updated to use Cluster class.
  5. Unit tests and MySQL integ tests have been changed and passed.


public String getNamespace() {
return namespace;

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra line

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

if (namespace != null ? !namespace.equals(cluster.namespace) : cluster.namespace != null)
return false;
return name != null ? name.equals(cluster.name) : cluster.name == null;

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra line

public Response findTasks(@QueryParam("namespace") Optional<String> namespace,
@QueryParam("clusterName") Optional<String> clusterName,
@QueryParam("state") Optional<TaskState> state) {
List<Task> result = taskQueue.peekTasks(new Cluster(namespace.get(), clusterName.get()),
state.map(TaskState::intValue).orElse(null));
Copy link
Contributor

@ywang282 ywang282 Aug 9, 2017

Choose a reason for hiding this comment

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

what if namespace and clusterName is not set in the query parameter?

get(): If a value is present in this Optional, returns the value, otherwise throws NoSuchElementException.

Copy link
Member Author

Choose a reason for hiding this comment

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

It should return all tasks.
will add some tests to verify

@shuzhang1989
Copy link
Member Author

@ywang282 it's work in progress, i will let you know when it's ready to review

@shuzhang1989 shuzhang1989 changed the title [WIP] Add cluster namespace support Add cluster namespace support Aug 12, 2017
* @author shu (shu@pinterest.com)
*/
public class Cluster {
public String namespace;
Copy link
Contributor

Choose a reason for hiding this comment

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

private?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

*/
public class Cluster {
public String namespace;
public String name;
Copy link
Contributor

Choose a reason for hiding this comment

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

also can we add some comments on what is namespace and what is name in the cluster?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure

@shuzhang1989 shuzhang1989 merged commit c5805c0 into pinterest:master Aug 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants