You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2021/09/08 17:40:32 UTC
[GitHub] [beam] lukecwik commented on a change in pull request #15464: [BEAM-12634] JmsIO auto scaling feature
lukecwik commented on a change in pull request #15464:
URL: https://github.com/apache/beam/pull/15464#discussion_r704625466
##########
File path: sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/AutoScaler.java
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.beam.sdk.io.jms;
+
+import java.io.Serializable;
+
+/**
+ * An interface allowing the implementation of getBacklogBytes and getSplitBacklogBytes.
Review comment:
```suggestion
* Enables users to specify their own `JMS` backlog reporters enabling {@link JmsIO} to report {@link UnboundedSource#getTotalBacklogBytes}.
```
##########
File path: sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/AutoScaler.java
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.beam.sdk.io.jms;
+
+import java.io.Serializable;
+
+/**
+ * An interface allowing the implementation of getBacklogBytes and getSplitBacklogBytes.
+ *
+ * <p>The interface also provides the following methods: - start to start the autoscaler - stop to
+ * stop the autoscaler
Review comment:
```suggestion
```
##########
File path: sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/AutoScaler.java
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.beam.sdk.io.jms;
+
+import java.io.Serializable;
+
+/**
+ * An interface allowing the implementation of getBacklogBytes and getSplitBacklogBytes.
+ *
+ * <p>The interface also provides the following methods: - start to start the autoscaler - stop to
+ * stop the autoscaler
+ */
+public interface AutoScaler extends Serializable {
+
+ void start();
Review comment:
```suggestion
/**
* The {@link AutoScaler} is started when the {@link UnboundedJmsReader} is started.
*/
void start();
```
##########
File path: sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/AutoScaler.java
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.beam.sdk.io.jms;
+
+import java.io.Serializable;
+
+/**
+ * An interface allowing the implementation of getBacklogBytes and getSplitBacklogBytes.
+ *
+ * <p>The interface also provides the following methods: - start to start the autoscaler - stop to
+ * stop the autoscaler
+ */
+public interface AutoScaler extends Serializable {
+
+ void start();
+
+ long getTotalBacklogBytes();
Review comment:
Copy the comment from `UnboundedSource#getTotalBacklogBytes`
##########
File path: sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/DefaultAutoscaler.java
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.beam.sdk.io.jms;
+
+import static org.apache.beam.sdk.io.UnboundedSource.UnboundedReader.BACKLOG_UNKNOWN;
+
+/**
+ * Default implementation of {@link AutoScaler}. Returns {@link
+ * org.apache.beam.sdk.io.UnboundedSource.UnboundedReader#BACKLOG_UNKNOWN} as default value for both
+ * getTotalBacklogBytes and getSplitBacklogBytes.
Review comment:
```suggestion
* org.apache.beam.sdk.io.UnboundedSource.UnboundedReader#BACKLOG_UNKNOWN} as the default value.
```
##########
File path: sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/AutoScaler.java
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.beam.sdk.io.jms;
+
+import java.io.Serializable;
+
+/**
+ * An interface allowing the implementation of getBacklogBytes and getSplitBacklogBytes.
+ *
+ * <p>The interface also provides the following methods: - start to start the autoscaler - stop to
+ * stop the autoscaler
+ */
+public interface AutoScaler extends Serializable {
+
+ void start();
+
+ long getTotalBacklogBytes();
+
+ long getSplitBacklogBytes();
+
+ void stop();
Review comment:
```suggestion
/**
* The {@link AutoScaler} is stopped when the {@link UnboundedJmsReader} is closed.
*/
void stop();
```
##########
File path: sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/JmsIO.java
##########
@@ -344,6 +348,11 @@ public static Write write() {
return builder().setCoder(coder).build();
}
+ public Read<T> withAutoScaler(AutoScaler autoScaler) {
Review comment:
```suggestion
/**
* Sets the {@link AutoScaler} to use for reporting backlog during the execution of this source.
*/
public Read<T> withAutoScaler(AutoScaler autoScaler) {
```
##########
File path: sdks/java/io/jms/src/main/java/org/apache/beam/sdk/io/jms/AutoScaler.java
##########
@@ -0,0 +1,37 @@
+/*
+ * 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.beam.sdk.io.jms;
+
+import java.io.Serializable;
+
+/**
+ * An interface allowing the implementation of getBacklogBytes and getSplitBacklogBytes.
+ *
+ * <p>The interface also provides the following methods: - start to start the autoscaler - stop to
+ * stop the autoscaler
+ */
+public interface AutoScaler extends Serializable {
+
+ void start();
+
+ long getTotalBacklogBytes();
+
+ long getSplitBacklogBytes();
Review comment:
I think we should drop this method since we have no way to provide to the scaler with what `split` we are currently processing since in the future it could change over time.
```suggestion
```
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org