Skip to content

Subscription: Push consumer & Async commit#12294

Merged
SteveYurongSu merged 10 commits intoapache:masterfrom
DanielWang2035:subs-push-consumer
Apr 19, 2024
Merged

Subscription: Push consumer & Async commit#12294
SteveYurongSu merged 10 commits intoapache:masterfrom
DanielWang2035:subs-push-consumer

Conversation

@DanielWang2035
Copy link
Copy Markdown
Collaborator

No description provided.

@SteveYurongSu SteveYurongSu self-assigned this Apr 3, 2024
# Conflicts:
#	iotdb-client/session/src/main/java/org/apache/iotdb/session/subscription/SubscriptionConsumer.java
#	iotdb-client/session/src/main/java/org/apache/iotdb/session/subscription/SubscriptionPullConsumer.java
#	iotdb-client/session/src/main/java/org/apache/iotdb/session/subscription/SubscriptionPushConsumer.java
Copy link
Copy Markdown
Contributor

@VGalaxies VGalaxies left a comment

Choose a reason for hiding this comment

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

some suggestions~

Comment on lines +59 to +60
public static final int PUSH_CONSUMER_AUTO_POLL_INTERVAL = 1000;
public static final int PUSH_CONSUMER_AUTO_POLL_TIME_OUT = 2000;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's better to add time units in the naming like _MS_.

@@ -174,7 +179,7 @@ public synchronized void close() throws IoTDBConnectionException {
shutdownEndpointsSyncer();

// shutdown heartbeat worker
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Correspondingly modify the comments.

import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;

// TODO
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove TODO?


// TODO
public static final String ACK_STRATEGY_KEY = "ack-strategy";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Need to support specifying ConsumeListener in the property-style constructor.

@SteveYurongSu SteveYurongSu changed the title Subscription: Push consumer Subscription: Push consumer & Async commit Apr 19, 2024
Copy link
Copy Markdown
Member

@SteveYurongSu SteveYurongSu left a comment

Choose a reason for hiding this comment

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

Overall LGTM

@SteveYurongSu SteveYurongSu merged commit 7004e4d into apache:master Apr 19, 2024
SzyWilliam pushed a commit to SzyWilliam/iotdb that referenced this pull request Nov 26, 2024
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.

3 participants