You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "wanghangyu817 (via GitHub)" <gi...@apache.org> on 2023/11/23 07:23:57 UTC
[PR] NIFI-12405 KeepAlive and KeepAlive Interval are set through configuration options [nifi]
wanghangyu817 opened a new pull request, #8061:
URL: https://github.com/apache/nifi/pull/8061
<!-- 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. -->
# Summary
[NIFI-12405](https://issues.apache.org/jira/browse/NIFI-12405)
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [ ] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [ ] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-12405`
- [ ] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-12405`
### Pull Request Formatting
- [ ] Pull Request based on current revision of the `main` branch
- [ ] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [ ] Build completed using `mvn clean install -P contrib-check`
- [ ] JDK 21
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12405 Fixed the problem that CaptureChangeMySQL uses BinaryLogClient to cause too many sessions [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory closed pull request #8061: NIFI-12405 Fixed the problem that CaptureChangeMySQL uses BinaryLogClient to cause too many sessions
URL: https://github.com/apache/nifi/pull/8061
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12405 Fixed the problem that CaptureChangeMySQL uses BinaryLogClient to cause too many sessions [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #8061:
URL: https://github.com/apache/nifi/pull/8061#discussion_r1410787708
##########
nifi-nar-bundles/nifi-cdc/nifi-cdc-mysql-bundle/nifi-cdc-mysql-processors/src/main/java/org/apache/nifi/cdc/mysql/processors/CaptureChangeMySQL.java:
##########
@@ -463,6 +463,39 @@ public class CaptureChangeMySQL extends AbstractSessionFactoryProcessor {
SSL_MODE_VERIFY_IDENTITY)
.build();
+ public static final PropertyDescriptor KEEP_ALIVE = new PropertyDescriptor.Builder()
+ .name("Keep Alive")
+ .displayName("Keep Alive")
+ .description("Used to specify heartbeat messages or queries to be sent after a connection has been idle for a period of time to ensure that " +
+ "the connection remains active. This prevents the database server or middleware from closing the " +
+ "connection on a connection that has been inactive for a long time")
+ .required(false)
+ .allowableValues("true", "false")
+ .defaultValue("true")
+ .addValidator(StandardValidators.BOOLEAN_VALIDATOR)
+ .build();
Review Comment:
Instead having this property, what do you think about just having the `Keep Alive Interval` property? Setting that value to `0` could effectively disable Keep Alive, and then this property would not be necessary.
##########
nifi-nar-bundles/nifi-cdc/nifi-cdc-mysql-bundle/nifi-cdc-mysql-processors/src/main/java/org/apache/nifi/cdc/mysql/processors/CaptureChangeMySQL.java:
##########
@@ -463,6 +463,39 @@ public class CaptureChangeMySQL extends AbstractSessionFactoryProcessor {
SSL_MODE_VERIFY_IDENTITY)
.build();
+ public static final PropertyDescriptor KEEP_ALIVE = new PropertyDescriptor.Builder()
+ .name("Keep Alive")
+ .displayName("Keep Alive")
+ .description("Used to specify heartbeat messages or queries to be sent after a connection has been idle for a period of time to ensure that " +
+ "the connection remains active. This prevents the database server or middleware from closing the " +
+ "connection on a connection that has been inactive for a long time")
+ .required(false)
+ .allowableValues("true", "false")
+ .defaultValue("true")
+ .addValidator(StandardValidators.BOOLEAN_VALIDATOR)
+ .build();
+
+ public static final PropertyDescriptor KEEPALIVE_INTERVAL = new PropertyDescriptor.Builder()
+ .name("KeepAlive Interval")
+ .displayName("KeepAlive Interval")
+ .description("Set the Keep-Alive interval in milliseconds for the connection. the default is 60 seconds")
+ .required(false)
+ .addValidator(StandardValidators.POSITIVE_LONG_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
+ .defaultValue("60000")
Review Comment:
Instead of using a number of milliseconds, this property can use the Time Period Validator, which supports different expressions, such as `60 s` or `500 ms`. That provides a more flexible configuration approach, which allowing the implementation code to get the value as milliseconds either way.
##########
nifi-nar-bundles/nifi-cdc/nifi-cdc-mysql-bundle/nifi-cdc-mysql-processors/src/main/java/org/apache/nifi/cdc/mysql/processors/CaptureChangeMySQL.java:
##########
@@ -692,7 +728,11 @@ public void setup(ProcessContext context) {
Long serverId = context.getProperty(SERVER_ID).evaluateAttributeExpressions().asLong();
- connect(hosts, username, password, serverId, driverLocation, driverName, connectTimeout, sslContextService, sslMode);
+ Boolean keepAlive = context.getProperty(KEEP_ALIVE).asBoolean();
+ Long keepAliveInterval = context.getProperty(KEEPALIVE_INTERVAL).evaluateAttributeExpressions().asLong();
+ Long heartbeatInterval = context.getProperty(HEARTBEAT_INTERVAL).evaluateAttributeExpressions().asLong();
Review Comment:
Recommend declaring these values as `final` and using the `long` primitive type. With these properties required, they should not be null.
##########
nifi-nar-bundles/nifi-cdc/nifi-cdc-mysql-bundle/nifi-cdc-mysql-processors/src/main/java/org/apache/nifi/cdc/mysql/processors/CaptureChangeMySQL.java:
##########
@@ -463,6 +463,39 @@ public class CaptureChangeMySQL extends AbstractSessionFactoryProcessor {
SSL_MODE_VERIFY_IDENTITY)
.build();
+ public static final PropertyDescriptor KEEP_ALIVE = new PropertyDescriptor.Builder()
+ .name("Keep Alive")
+ .displayName("Keep Alive")
+ .description("Used to specify heartbeat messages or queries to be sent after a connection has been idle for a period of time to ensure that " +
+ "the connection remains active. This prevents the database server or middleware from closing the " +
+ "connection on a connection that has been inactive for a long time")
+ .required(false)
+ .allowableValues("true", "false")
+ .defaultValue("true")
+ .addValidator(StandardValidators.BOOLEAN_VALIDATOR)
+ .build();
+
+ public static final PropertyDescriptor KEEPALIVE_INTERVAL = new PropertyDescriptor.Builder()
+ .name("KeepAlive Interval")
+ .displayName("KeepAlive Interval")
+ .description("Set the Keep-Alive interval in milliseconds for the connection. the default is 60 seconds")
+ .required(false)
Review Comment:
Since a default value is specified, this property can be marked as required.
##########
nifi-nar-bundles/nifi-cdc/nifi-cdc-mysql-bundle/nifi-cdc-mysql-processors/src/main/java/org/apache/nifi/cdc/mysql/processors/CaptureChangeMySQL.java:
##########
@@ -463,6 +463,39 @@ public class CaptureChangeMySQL extends AbstractSessionFactoryProcessor {
SSL_MODE_VERIFY_IDENTITY)
.build();
+ public static final PropertyDescriptor KEEP_ALIVE = new PropertyDescriptor.Builder()
+ .name("Keep Alive")
+ .displayName("Keep Alive")
+ .description("Used to specify heartbeat messages or queries to be sent after a connection has been idle for a period of time to ensure that " +
+ "the connection remains active. This prevents the database server or middleware from closing the " +
+ "connection on a connection that has been inactive for a long time")
+ .required(false)
+ .allowableValues("true", "false")
+ .defaultValue("true")
+ .addValidator(StandardValidators.BOOLEAN_VALIDATOR)
+ .build();
+
+ public static final PropertyDescriptor KEEPALIVE_INTERVAL = new PropertyDescriptor.Builder()
+ .name("KeepAlive Interval")
+ .displayName("KeepAlive Interval")
+ .description("Set the Keep-Alive interval in milliseconds for the connection. the default is 60 seconds")
+ .required(false)
+ .addValidator(StandardValidators.POSITIVE_LONG_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
+ .defaultValue("60000")
+ .build();
+
+ public static final PropertyDescriptor HEARTBEAT_INTERVAL = new PropertyDescriptor.Builder()
+ .name("Heartbeat Interval")
+ .displayName("Heartbeat Interval")
+ .description("heartbeatInterval – heartbeat period in milliseconds.the default is 1 seconds." +
+ "Note that when used together with keepAlive heartbeatInterval MUST be set less than keepAliveInterval.")
+ .required(false)
+ .addValidator(StandardValidators.POSITIVE_LONG_VALIDATOR)
+ .expressionLanguageSupported(ExpressionLanguageScope.ENVIRONMENT)
+ .defaultValue("1000")
Review Comment:
See note above regarding the use of the Time Period Validator.
##########
nifi-nar-bundles/nifi-cdc/nifi-cdc-mysql-bundle/nifi-cdc-mysql-processors/src/main/java/org/apache/nifi/cdc/mysql/processors/CaptureChangeMySQL.java:
##########
@@ -850,6 +890,10 @@ protected void connect(List<InetSocketAddress> hosts, String username, String pa
binlogClient.setSslSocketFactory(sslSocketFactory);
}
+ binlogClient.setKeepAlive(keepAlive);
Review Comment:
As mentioned above, based on using `0` as an indicator of disabled keep alive, this property could be set conditionally.
##########
nifi-nar-bundles/nifi-cdc/nifi-cdc-mysql-bundle/nifi-cdc-mysql-processors/src/main/java/org/apache/nifi/cdc/mysql/processors/CaptureChangeMySQL.java:
##########
@@ -463,6 +463,39 @@ public class CaptureChangeMySQL extends AbstractSessionFactoryProcessor {
SSL_MODE_VERIFY_IDENTITY)
.build();
+ public static final PropertyDescriptor KEEP_ALIVE = new PropertyDescriptor.Builder()
+ .name("Keep Alive")
+ .displayName("Keep Alive")
+ .description("Used to specify heartbeat messages or queries to be sent after a connection has been idle for a period of time to ensure that " +
+ "the connection remains active. This prevents the database server or middleware from closing the " +
+ "connection on a connection that has been inactive for a long time")
+ .required(false)
+ .allowableValues("true", "false")
+ .defaultValue("true")
+ .addValidator(StandardValidators.BOOLEAN_VALIDATOR)
+ .build();
+
+ public static final PropertyDescriptor KEEPALIVE_INTERVAL = new PropertyDescriptor.Builder()
+ .name("KeepAlive Interval")
+ .displayName("KeepAlive Interval")
+ .description("Set the Keep-Alive interval in milliseconds for the connection. the default is 60 seconds")
Review Comment:
It is not necessary to mention the default value in the description, but it would be worth mentioning that setting the value to 0 disables Keep Alive messages.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12405 Fixed the problem that CaptureChangeMySQL uses BinaryLogClient to cause too many sessions [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on PR #8061:
URL: https://github.com/apache/nifi/pull/8061#issuecomment-1947683856
@wanghangyu817 Thanks again for this contribution. As it has been several months, and there is now a merge conflict, I am closing this pull request for now. Please feel free to reopen the pull request after rebasing against the current main branch when you are ready to address the feedback provided.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org