[GitHub] [samza] Sanil15 commented on a change in pull request #1219: SAMZA-2373: [SEP-22] Container Placement Service (core functionality) for container move and restart
Sanil15 commented on a change in pull request #1219: SAMZA-2373: [SEP-22]
Container Placement Service (core functionality) for container move and restart
URL: https://github.com/apache/samza/pull/1219#discussion_r371040979
##
File path:
samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.java
##
@@ -202,12 +202,9 @@ void assignResourceRequests() {
String preferredHost = hostAffinityEnabled ? request.getPreferredHost()
: ResourceRequestState.ANY_HOST;
Instant requestCreationTime = request.getRequestTimestamp();
- LOG.info("Handling assignment request for Processor ID: {} on host:
{}.", processorId, preferredHost);
+ LOG.info("Handling assignment for request {}", request);
if (hasAllocatedResource(preferredHost)) {
-// Found allocated container on preferredHost
-LOG.info("Found an available container for Processor ID: {} on the
host: {}", processorId, preferredHost);
Review comment:
note: hasAllocatedResource(...) already logs this line so this is not userful
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [samza] Sanil15 commented on a change in pull request #1219: SAMZA-2373: [SEP-22] Container Placement Service (core functionality) for container move and restart
Sanil15 commented on a change in pull request #1219: SAMZA-2373: [SEP-22]
Container Placement Service (core functionality) for container move and restart
URL: https://github.com/apache/samza/pull/1219#discussion_r371040979
##
File path:
samza-core/src/main/java/org/apache/samza/clustermanager/ContainerAllocator.java
##
@@ -202,12 +202,9 @@ void assignResourceRequests() {
String preferredHost = hostAffinityEnabled ? request.getPreferredHost()
: ResourceRequestState.ANY_HOST;
Instant requestCreationTime = request.getRequestTimestamp();
- LOG.info("Handling assignment request for Processor ID: {} on host:
{}.", processorId, preferredHost);
+ LOG.info("Handling assignment for request {}", request);
if (hasAllocatedResource(preferredHost)) {
-// Found allocated container on preferredHost
-LOG.info("Found an available container for Processor ID: {} on the
host: {}", processorId, preferredHost);
Review comment:
note: hasAllocatedResource(...) already logs this line so this is not userful
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [samza] Sanil15 commented on a change in pull request #1219: SAMZA-2373: [SEP-22] Container Placement Service (core functionality) for container move and restart
Sanil15 commented on a change in pull request #1219: SAMZA-2373: [SEP-22]
Container Placement Service (core functionality) for container move and restart
URL: https://github.com/apache/samza/pull/1219#discussion_r366489208
##
File path:
samza-api/src/main/java/org/apache/samza/container/placements/ContainerPlacementMessage.java
##
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.samza.container.placements;
+
+import com.google.common.base.Preconditions;
+import java.time.Duration;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.UUID;
+
+
+/**
+ * Encapsulates the request or response payload information between the
ContainerPlacementHandler service and external
+ * controllers issuing placement actions
+ */
+public abstract class ContainerPlacementMessage {
+
+ public enum StatusCode {
+/**
+ * Indicates that the container placement action is created
+ */
+CREATED,
+
+/**
+ * Indicates that the container placement action was rejected because
request was deemed invalid
+ */
+BAD_REQUEST,
+
+/**
+ * Indicates that the container placement action is accepted and waiting
to be processed
+ */
+ACCEPTED,
+
+/**
+ * Indicates that the container placement action is in progress
+ */
+IN_PROGRESS,
+
+/**
+ * Indicates that the container placement action is in progress
+ */
+SUCCEEDED,
+
+/**
+ * Indicates that the container placement action is in failed
+ */
+FAILED;
+ }
+
+ /**
+ * UUID attached to a message which helps in identifying duplicate request
messages written to metastore and not
+ * retake actions even if metastore is eventually consistent
+ */
+ protected final UUID uuid;
+ /**
+ * Unique identifier for a deployment so messages can be invalidated across
a job restarts
+ * for ex yarn bases cluster manager should set this to app attempt id
+ */
+ protected final String applicationId;
+ // Logical container Id 0, 1, 2
+ protected final String processorId;
+ // Destination host where container is desired to be moved
+ protected final String destinationHost;
+ // Optional request expiry which acts as a timeout for any resource request
to cluster manager
+ protected final Optional requestExpiryTimeout;
+ // Status of the current request
+ protected final StatusCode statusCode;
+
+ protected ContainerPlacementMessage(UUID uuid, String applicationId, String
processorId, String destinationHost,
+ Duration requestExpiryTimeout, StatusCode statusCode) {
+Preconditions.checkNotNull(uuid, "uuid cannot be null");
+Preconditions.checkNotNull(applicationId, "applicationId cannot be null");
+Preconditions.checkNotNull(processorId, "processorId cannot be null");
+Preconditions.checkNotNull(destinationHost, "destinationHost cannot be
null");
+Preconditions.checkNotNull(statusCode, "statusCode cannot be null");
+this.uuid = uuid;
Review comment:
Sure but for consistency with other code I prefer to use Preconditions,
https://stackoverflow.com/questions/34646554/java-util-objects-requirenonnull-vs-preconditions-checknotnull
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [samza] Sanil15 commented on a change in pull request #1219: SAMZA-2373: [SEP-22] Container Placement Service (core functionality) for container move and restart
Sanil15 commented on a change in pull request #1219: SAMZA-2373: [SEP-22]
Container Placement Service (core functionality) for container move and restart
URL: https://github.com/apache/samza/pull/1219#discussion_r366488160
##
File path:
samza-api/src/main/java/org/apache/samza/container/placements/ContainerPlacementRequestMessage.java
##
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.samza.container.placements;
+
+import java.time.Duration;
+import java.util.UUID;
+
+
+/**
+ * Encapsulates the request sent from the external controller to the
JobCoordinator to take a container placement action
+ */
+public class ContainerPlacementRequestMessage extends
ContainerPlacementMessage {
+
+ public ContainerPlacementRequestMessage(UUID uuid, String applicationId,
String processorId, String destinationHost, Duration requestExpiryTimeout) {
+super(uuid, applicationId, processorId, destinationHost,
requestExpiryTimeout, StatusCode.CREATED);
+ }
+
+ public ContainerPlacementRequestMessage(UUID uuid, String applicationId,
String processorId, String destinationHost) {
+super(uuid, applicationId, processorId, destinationHost,
StatusCode.CREATED);
+ }
+
+ @Override
+ public String toString() {
+return "ContainerPlacementRequestMessage{" + "uuid=" + uuid + ",
applicationId='" + applicationId + '\''
++ ", processorId='" + processorId + '\'' + ", destinationHost='" +
destinationHost + '\'' + ", requestExpiryTimeout="
++ requestExpiryTimeout + ", statusCode=" + statusCode + "} " +
super.toString();
+ }
+
+ @Override
+ public int hashCode() {
+return super.hashCode();
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+return super.equals(obj);
+ }
Review comment:
removing it here: https://github.com/apache/samza/pull/1227
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [samza] Sanil15 commented on a change in pull request #1219: SAMZA-2373: [SEP-22] Container Placement Service (core functionality) for container move and restart
Sanil15 commented on a change in pull request #1219: SAMZA-2373: [SEP-22]
Container Placement Service (core functionality) for container move and restart
URL: https://github.com/apache/samza/pull/1219#discussion_r366487988
##
File path:
samza-api/src/main/java/org/apache/samza/container/placements/ContainerPlacementMessage.java
##
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.samza.container.placements;
+
+import com.google.common.base.Preconditions;
+import java.time.Duration;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.UUID;
+
+
+/**
+ * Encapsulates the request or response payload information between the
ContainerPlacementHandler service and external
+ * controllers issuing placement actions
+ */
+public abstract class ContainerPlacementMessage {
+
+ public enum StatusCode {
+/**
+ * Indicates that the container placement action is created
+ */
+CREATED,
+
+/**
+ * Indicates that the container placement action was rejected because
request was deemed invalid
+ */
+BAD_REQUEST,
+
+/**
+ * Indicates that the container placement action is accepted and waiting
to be processed
+ */
+ACCEPTED,
+
+/**
+ * Indicates that the container placement action is in progress
+ */
+IN_PROGRESS,
+
+/**
+ * Indicates that the container placement action is in progress
+ */
+SUCCEEDED,
+
+/**
+ * Indicates that the container placement action is in failed
+ */
+FAILED;
+ }
+
+ /**
+ * UUID attached to a message which helps in identifying duplicate request
messages written to metastore and not
+ * retake actions even if metastore is eventually consistent
+ */
+ protected final UUID uuid;
+ /**
+ * Unique identifier for a deployment so messages can be invalidated across
a job restarts
+ * for ex yarn bases cluster manager should set this to app attempt id
+ */
+ protected final String applicationId;
+ // Logical container Id 0, 1, 2
+ protected final String processorId;
+ // Destination host where container is desired to be moved
+ protected final String destinationHost;
+ // Optional request expiry which acts as a timeout for any resource request
to cluster manager
+ protected final Optional requestExpiryTimeout;
+ // Status of the current request
+ protected final StatusCode statusCode;
+
+ protected ContainerPlacementMessage(UUID uuid, String applicationId, String
processorId, String destinationHost,
+ Duration requestExpiryTimeout, StatusCode statusCode) {
+Preconditions.checkNotNull(uuid, "uuid cannot be null");
+Preconditions.checkNotNull(applicationId, "applicationId cannot be null");
+Preconditions.checkNotNull(processorId, "processorId cannot be null");
+Preconditions.checkNotNull(destinationHost, "destinationHost cannot be
null");
+Preconditions.checkNotNull(statusCode, "statusCode cannot be null");
+this.uuid = uuid;
+this.applicationId = applicationId;
+this.processorId = processorId;
+this.destinationHost = destinationHost;
+this.requestExpiryTimeout = Optional.ofNullable(requestExpiryTimeout);
+this.statusCode = statusCode;
+ }
+
+ protected ContainerPlacementMessage(UUID uuid, String applicationId, String
processorId, String destinationHost,
+ StatusCode statusCode) {
+this(uuid, applicationId, processorId, destinationHost, null, statusCode);
+ }
+
+ public String getApplicationId() {
+return applicationId;
+ }
+
+ public String getProcessorId() {
+return processorId;
+ }
+
+ public String getDestinationHost() {
+return destinationHost;
+ }
+
+ public StatusCode getStatusCode() {
+return statusCode;
+ }
+
+ public Optional getRequestExpiryTimeout() {
+return requestExpiryTimeout;
+ }
+
+ public UUID getUuid() {
+return uuid;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+if (this == o) {
+ return true;
+}
+if (o == null || getClass() != o.getClass()) {
+ return false;
+}
+ContainerPlacementMessage that = (ContainerPlacementMessage) o;
+return uuid.equals(that.uuid) &&
getApplicationId().e
[GitHub] [samza] Sanil15 commented on a change in pull request #1219: SAMZA-2373: [SEP-22] Container Placement Service (core functionality) for container move and restart
Sanil15 commented on a change in pull request #1219: SAMZA-2373: [SEP-22]
Container Placement Service (core functionality) for container move and restart
URL: https://github.com/apache/samza/pull/1219#discussion_r366487988
##
File path:
samza-api/src/main/java/org/apache/samza/container/placements/ContainerPlacementMessage.java
##
@@ -0,0 +1,147 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.samza.container.placements;
+
+import com.google.common.base.Preconditions;
+import java.time.Duration;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.UUID;
+
+
+/**
+ * Encapsulates the request or response payload information between the
ContainerPlacementHandler service and external
+ * controllers issuing placement actions
+ */
+public abstract class ContainerPlacementMessage {
+
+ public enum StatusCode {
+/**
+ * Indicates that the container placement action is created
+ */
+CREATED,
+
+/**
+ * Indicates that the container placement action was rejected because
request was deemed invalid
+ */
+BAD_REQUEST,
+
+/**
+ * Indicates that the container placement action is accepted and waiting
to be processed
+ */
+ACCEPTED,
+
+/**
+ * Indicates that the container placement action is in progress
+ */
+IN_PROGRESS,
+
+/**
+ * Indicates that the container placement action is in progress
+ */
+SUCCEEDED,
+
+/**
+ * Indicates that the container placement action is in failed
+ */
+FAILED;
+ }
+
+ /**
+ * UUID attached to a message which helps in identifying duplicate request
messages written to metastore and not
+ * retake actions even if metastore is eventually consistent
+ */
+ protected final UUID uuid;
+ /**
+ * Unique identifier for a deployment so messages can be invalidated across
a job restarts
+ * for ex yarn bases cluster manager should set this to app attempt id
+ */
+ protected final String applicationId;
+ // Logical container Id 0, 1, 2
+ protected final String processorId;
+ // Destination host where container is desired to be moved
+ protected final String destinationHost;
+ // Optional request expiry which acts as a timeout for any resource request
to cluster manager
+ protected final Optional requestExpiryTimeout;
+ // Status of the current request
+ protected final StatusCode statusCode;
+
+ protected ContainerPlacementMessage(UUID uuid, String applicationId, String
processorId, String destinationHost,
+ Duration requestExpiryTimeout, StatusCode statusCode) {
+Preconditions.checkNotNull(uuid, "uuid cannot be null");
+Preconditions.checkNotNull(applicationId, "applicationId cannot be null");
+Preconditions.checkNotNull(processorId, "processorId cannot be null");
+Preconditions.checkNotNull(destinationHost, "destinationHost cannot be
null");
+Preconditions.checkNotNull(statusCode, "statusCode cannot be null");
+this.uuid = uuid;
+this.applicationId = applicationId;
+this.processorId = processorId;
+this.destinationHost = destinationHost;
+this.requestExpiryTimeout = Optional.ofNullable(requestExpiryTimeout);
+this.statusCode = statusCode;
+ }
+
+ protected ContainerPlacementMessage(UUID uuid, String applicationId, String
processorId, String destinationHost,
+ StatusCode statusCode) {
+this(uuid, applicationId, processorId, destinationHost, null, statusCode);
+ }
+
+ public String getApplicationId() {
+return applicationId;
+ }
+
+ public String getProcessorId() {
+return processorId;
+ }
+
+ public String getDestinationHost() {
+return destinationHost;
+ }
+
+ public StatusCode getStatusCode() {
+return statusCode;
+ }
+
+ public Optional getRequestExpiryTimeout() {
+return requestExpiryTimeout;
+ }
+
+ public UUID getUuid() {
+return uuid;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+if (this == o) {
+ return true;
+}
+if (o == null || getClass() != o.getClass()) {
+ return false;
+}
+ContainerPlacementMessage that = (ContainerPlacementMessage) o;
+return uuid.equals(that.uuid) &&
getApplicationId().e
[GitHub] [samza] Sanil15 commented on a change in pull request #1219: SAMZA-2373: [SEP-22] Container Placement Service (core functionality) for container move and restart
Sanil15 commented on a change in pull request #1219: SAMZA-2373: [SEP-22]
Container Placement Service (core functionality) for container move and restart
URL: https://github.com/apache/samza/pull/1219#discussion_r366488055
##
File path:
samza-api/src/main/java/org/apache/samza/container/placements/ContainerPlacementRequestMessage.java
##
@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.samza.container.placements;
+
+import java.time.Duration;
+import java.util.UUID;
+
+
+/**
+ * Encapsulates the request sent from the external controller to the
JobCoordinator to take a container placement action
+ */
+public class ContainerPlacementRequestMessage extends
ContainerPlacementMessage {
+
+ public ContainerPlacementRequestMessage(UUID uuid, String applicationId,
String processorId, String destinationHost, Duration requestExpiryTimeout) {
+super(uuid, applicationId, processorId, destinationHost,
requestExpiryTimeout, StatusCode.CREATED);
+ }
+
+ public ContainerPlacementRequestMessage(UUID uuid, String applicationId,
String processorId, String destinationHost) {
+super(uuid, applicationId, processorId, destinationHost,
StatusCode.CREATED);
+ }
+
+ @Override
+ public String toString() {
+return "ContainerPlacementRequestMessage{" + "uuid=" + uuid + ",
applicationId='" + applicationId + '\''
++ ", processorId='" + processorId + '\'' + ", destinationHost='" +
destinationHost + '\'' + ", requestExpiryTimeout="
++ requestExpiryTimeout + ", statusCode=" + statusCode + "} " +
super.toString();
Review comment:
Already used here: https://github.com/apache/samza/pull/1227
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
