You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@samza.apache.org by "mynameborat (via GitHub)" <gi...@apache.org> on 2023/08/03 07:48:32 UTC

[GitHub] [samza] mynameborat opened a new pull request, #1679: SAMZA-2789: Remove cap on intermediate stream partition count for stream mode

mynameborat opened a new pull request, #1679:
URL: https://github.com/apache/samza/pull/1679

   **Problem**: Intermediate stream partition count inference logic caps the partition size to 256 resulting in imbalances in work assignments to tasks
   
   **Description**: As part of the intermediate partition size inference logic, we currently employ the following algorithm.
   
   - partitionCount = Math.max(maxPartitionSize(inputStreams), maxPartitionSize(outputStreams))
   - cap the partitionCount to MAX_INFERRED_PARTITIONS defined in the `IntermediateStreamManager` which is 256
   - apply the inferred partition count to intermediate streams whose partition count is uninitialized
   
   The logic above always caps the partition size of intermediate streams to 256 for all auto-created intermediate streams. This can prevent the job from scaling up uniformly as the intermediate partition assignment is capped to 256 tasks thereby rendering other tasks imbalanced in case of number tasks > 256.
   
   **Changes**:
   - Apply the cap only for batch mode as 256 limit was introduced for batch mode where number of files (partition) could be large
   - Add unit tests for `IntermediateStreamManager`
   - Minor java doc fix for `DefaultTaskExecutorFactory`
   
   **Tests**: Added unit tests for the code changes
   
   **API Changes**: None
   
   **Upgrade Instructions**:
   - Jobs that are temporarily worked around this constraint by setting `job.intermediate.stream.partitions` should remove the configuration in order for samza to infer and apply the partition count as described above
   - Jobs that don't use `job.intermediate.stream.partitions` need no changes.
   
   **Usage Instructions**: Refer to upgrade instruction.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [samza] mynameborat commented on a diff in pull request #1679: SAMZA-2789: Remove cap on intermediate stream partition count for stream mode

Posted by "mynameborat (via GitHub)" <gi...@apache.org>.
mynameborat commented on code in PR #1679:
URL: https://github.com/apache/samza/pull/1679#discussion_r1284234100


##########
samza-core/src/main/java/org/apache/samza/task/DefaultTaskExecutorFactory.java:
##########
@@ -50,9 +50,8 @@ public ExecutorService getTaskExecutor(Config config) {
    * {@inheritDoc}
    *
    * The choice of thread pool is determined based on the following logic
-   *    If job.operator.thread.pool.enabled,
-   *     a. Use {@link #getTaskExecutor(Config)} if job.container.thread.pool.size &gt; 1
-   *     b. Use default single threaded pool otherwise
+   *     1. Use {@link #getTaskExecutor(Config)} if job.container.thread.pool.size &gt; 1

Review Comment:
   Yes. but I didn't want to open a separate PR for this one line java doc change.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [samza] mynameborat merged pull request #1679: SAMZA-2789: Remove cap on intermediate stream partition count for stream mode

Posted by "mynameborat (via GitHub)" <gi...@apache.org>.
mynameborat merged PR #1679:
URL: https://github.com/apache/samza/pull/1679


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [samza] ajothomas commented on a diff in pull request #1679: SAMZA-2789: Remove cap on intermediate stream partition count for stream mode

Posted by "ajothomas (via GitHub)" <gi...@apache.org>.
ajothomas commented on code in PR #1679:
URL: https://github.com/apache/samza/pull/1679#discussion_r1283830982


##########
samza-core/src/main/java/org/apache/samza/task/DefaultTaskExecutorFactory.java:
##########
@@ -50,9 +50,8 @@ public ExecutorService getTaskExecutor(Config config) {
    * {@inheritDoc}
    *
    * The choice of thread pool is determined based on the following logic
-   *    If job.operator.thread.pool.enabled,
-   *     a. Use {@link #getTaskExecutor(Config)} if job.container.thread.pool.size &gt; 1
-   *     b. Use default single threaded pool otherwise
+   *     1. Use {@link #getTaskExecutor(Config)} if job.container.thread.pool.size &gt; 1

Review Comment:
   Nit: This change seems unrelated and doesn't affect build. Do we need this with this PR?



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [samza] alnzng commented on pull request #1679: SAMZA-2789: Remove cap on intermediate stream partition count for stream mode

Posted by "alnzng (via GitHub)" <gi...@apache.org>.
alnzng commented on PR #1679:
URL: https://github.com/apache/samza/pull/1679#issuecomment-1664778968

   LGTM, thanks for making this change.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [samza] dxichen commented on a diff in pull request #1679: SAMZA-2789: Remove cap on intermediate stream partition count for stream mode

Posted by "dxichen (via GitHub)" <gi...@apache.org>.
dxichen commented on code in PR #1679:
URL: https://github.com/apache/samza/pull/1679#discussion_r1283831901


##########
samza-core/src/test/java/org/apache/samza/execution/TestIntermediateStreamManager.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * 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.execution;
+
+import com.google.common.collect.ImmutableSet;
+import org.apache.samza.config.ApplicationConfig.ApplicationMode;
+import org.apache.samza.config.Config;
+import org.apache.samza.config.JobConfig;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mock;
+
+import org.mockito.MockitoAnnotations;
+import static org.mockito.Mockito.*;

Review Comment:
   Remove wildcard imports



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [samza] mynameborat commented on a diff in pull request #1679: SAMZA-2789: Remove cap on intermediate stream partition count for stream mode

Posted by "mynameborat (via GitHub)" <gi...@apache.org>.
mynameborat commented on code in PR #1679:
URL: https://github.com/apache/samza/pull/1679#discussion_r1284237393


##########
samza-core/src/test/java/org/apache/samza/execution/TestIntermediateStreamManager.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * 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.execution;
+
+import com.google.common.collect.ImmutableSet;
+import org.apache.samza.config.ApplicationConfig.ApplicationMode;
+import org.apache.samza.config.Config;
+import org.apache.samza.config.JobConfig;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mock;
+
+import org.mockito.MockitoAnnotations;
+import static org.mockito.Mockito.*;

Review Comment:
   The convention in the code base is to allow wildcards only for static imports. Agree on non-wildcard for non-static imports.



-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@samza.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org