You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/11/24 07:27:48 UTC
[GitHub] [pinot] navina opened a new pull request, #9859: Use Pulsar AdminClient to delete unused subscriptions
navina opened a new pull request, #9859:
URL: https://github.com/apache/pinot/pull/9859
`PulsarStreamMetadataProvider` creates a `Consumer` instance with a random subscription name to fetch stream related metadata. However, this subscription is not deleted when the consumer is closed. This subscription is not really shared across pinot consumer sessions and thus, can be safely deleted.
Related issue: #9854
Labels: `bugfix`
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] navina commented on pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
navina commented on PR #9859:
URL: https://github.com/apache/pinot/pull/9859#issuecomment-1326060005
@KKcorps : please review.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] mathieudruart commented on a diff in pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
mathieudruart commented on code in PR #9859:
URL: https://github.com/apache/pinot/pull/9859#discussion_r1031698031
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarStreamMetadataProvider.java:
##########
@@ -156,11 +174,21 @@ public List<PartitionGroupMetadata> computePartitionGroupMetadata(String clientI
LOGGER.warn("Error encountered while calculating pulsar partition group metadata: " + e.getMessage(), e);
} finally {
closeConsumer(consumer);
+ deleteSubscription(_topic, subscription);
}
return newPartitionGroupMetadataList;
}
+ private void deleteSubscription(String topicName, String subscription) {
+ try {
+ _pulsarAdminClient.topics().deleteSubscription(topicName, subscription);
Review Comment:
I saw that it was possible to create lightweight subscriptions using non durable subscription mode, it may be a simpler / more efficient solution to solve the problem (the subscription is never persisted):
Consumer<byte[]> consumer = pulsarClient.newConsumer()
.topic("my-topic")
.subscriptionName("my-sub")
.subscriptionMode(SubscriptionMode.NonDurable)
.subscribe();
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] mathieudruart commented on a diff in pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
mathieudruart commented on code in PR #9859:
URL: https://github.com/apache/pinot/pull/9859#discussion_r1034020915
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarStreamMetadataProvider.java:
##########
@@ -156,11 +174,21 @@ public List<PartitionGroupMetadata> computePartitionGroupMetadata(String clientI
LOGGER.warn("Error encountered while calculating pulsar partition group metadata: " + e.getMessage(), e);
} finally {
closeConsumer(consumer);
+ deleteSubscription(_topic, subscription);
}
return newPartitionGroupMetadataList;
}
+ private void deleteSubscription(String topicName, String subscription) {
+ try {
+ _pulsarAdminClient.topics().deleteSubscription(topicName, subscription);
Review Comment:
@navina non durable subscriptions are not persisted, so I suppose there are no cursors written on BookKeeper. I assume, without having checked, that this makes it possible to create and delete subscriptions more quickly.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] mathieudruart commented on a diff in pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
mathieudruart commented on code in PR #9859:
URL: https://github.com/apache/pinot/pull/9859#discussion_r1031698031
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarStreamMetadataProvider.java:
##########
@@ -156,11 +174,21 @@ public List<PartitionGroupMetadata> computePartitionGroupMetadata(String clientI
LOGGER.warn("Error encountered while calculating pulsar partition group metadata: " + e.getMessage(), e);
} finally {
closeConsumer(consumer);
+ deleteSubscription(_topic, subscription);
}
return newPartitionGroupMetadataList;
}
+ private void deleteSubscription(String topicName, String subscription) {
+ try {
+ _pulsarAdminClient.topics().deleteSubscription(topicName, subscription);
Review Comment:
I saw that it was possible to create lightweight subscriptions using non durable subscription mode, it may be a simpler / more efficient solution to solve the problem (the subscription is never persisted):
Consumer<byte[]> consumer = pulsarClient.newConsumer()
.topic("my-topic")
.subscriptionName("my-sub")
.subscriptionMode(SubscriptionMode.NonDurable)
.subscribe();
in this case the subscription is still stored in the Pulsar broker memory (but no longer persisted with a durable cursor).
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] navina commented on a diff in pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
navina commented on code in PR #9859:
URL: https://github.com/apache/pinot/pull/9859#discussion_r1036323485
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarStreamMetadataProvider.java:
##########
@@ -156,11 +174,21 @@ public List<PartitionGroupMetadata> computePartitionGroupMetadata(String clientI
LOGGER.warn("Error encountered while calculating pulsar partition group metadata: " + e.getMessage(), e);
} finally {
closeConsumer(consumer);
+ deleteSubscription(_topic, subscription);
}
return newPartitionGroupMetadataList;
}
+ private void deleteSubscription(String topicName, String subscription) {
+ try {
+ _pulsarAdminClient.topics().deleteSubscription(topicName, subscription);
Review Comment:
>I haven't tested, but maybe a non-durable subscription doesn't need to be deleted? maybe it is automatically deleted once consumer is closed?
Ok. I will test it today as I have some time :)
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] mathieudruart commented on a diff in pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
mathieudruart commented on code in PR #9859:
URL: https://github.com/apache/pinot/pull/9859#discussion_r1031698031
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarStreamMetadataProvider.java:
##########
@@ -156,11 +174,21 @@ public List<PartitionGroupMetadata> computePartitionGroupMetadata(String clientI
LOGGER.warn("Error encountered while calculating pulsar partition group metadata: " + e.getMessage(), e);
} finally {
closeConsumer(consumer);
+ deleteSubscription(_topic, subscription);
}
return newPartitionGroupMetadataList;
}
+ private void deleteSubscription(String topicName, String subscription) {
+ try {
+ _pulsarAdminClient.topics().deleteSubscription(topicName, subscription);
Review Comment:
I saw that it was possible to create lightweight subscriptions using non durable subscription mode, it may be a simpler / more efficient solution to solve the problem (the subscription is never persisted):
Consumer<byte[]> consumer = pulsarClient.newConsumer()
.topic("my-topic")
.subscriptionName("my-sub")
.subscriptionMode(SubscriptionMode.NonDurable)
.subscribe();
in this case the subscription is still stored in the Pulsar broker memory (but no longer persisted with a durable cursor).
I think we should create them non-durable, but maybe also delete them to clean the memory of the Pulsar brokers.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] eladar2000 commented on a diff in pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
eladar2000 commented on code in PR #9859:
URL: https://github.com/apache/pinot/pull/9859#discussion_r1034019910
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarStreamMetadataProvider.java:
##########
@@ -156,11 +174,21 @@ public List<PartitionGroupMetadata> computePartitionGroupMetadata(String clientI
LOGGER.warn("Error encountered while calculating pulsar partition group metadata: " + e.getMessage(), e);
} finally {
closeConsumer(consumer);
+ deleteSubscription(_topic, subscription);
}
return newPartitionGroupMetadataList;
}
+ private void deleteSubscription(String topicName, String subscription) {
+ try {
+ _pulsarAdminClient.topics().deleteSubscription(topicName, subscription);
Review Comment:
@navina non durable subscriptions are not persisted, so I suppose there are no cursors written on BookKeeper. I assume, without having checked, that this makes it possible to create and delete subscriptions more quickly.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang merged pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang merged PR #9859:
URL: https://github.com/apache/pinot/pull/9859
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] navina commented on a diff in pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
navina commented on code in PR #9859:
URL: https://github.com/apache/pinot/pull/9859#discussion_r1036471037
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarStreamMetadataProvider.java:
##########
@@ -156,11 +174,21 @@ public List<PartitionGroupMetadata> computePartitionGroupMetadata(String clientI
LOGGER.warn("Error encountered while calculating pulsar partition group metadata: " + e.getMessage(), e);
} finally {
closeConsumer(consumer);
+ deleteSubscription(_topic, subscription);
}
return newPartitionGroupMetadataList;
}
+ private void deleteSubscription(String topicName, String subscription) {
+ try {
+ _pulsarAdminClient.topics().deleteSubscription(topicName, subscription);
Review Comment:
Alright. I was able to verify with pulsar standalone mode that the non-durable subscription gets instantly deleted if the consumer is closed. 👏
@mathieudruart the actual root cause of all those unused subscriptions not getting cleaned up on the broker is due to the fact that we are not closing the consumer properly in the metadata provider. I have already fixed that in the PR.
Will update the PR to use NonDurable subscription mode and remove the explicit delete.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] navina commented on a diff in pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
navina commented on code in PR #9859:
URL: https://github.com/apache/pinot/pull/9859#discussion_r1035172757
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarStreamMetadataProvider.java:
##########
@@ -156,11 +174,21 @@ public List<PartitionGroupMetadata> computePartitionGroupMetadata(String clientI
LOGGER.warn("Error encountered while calculating pulsar partition group metadata: " + e.getMessage(), e);
} finally {
closeConsumer(consumer);
+ deleteSubscription(_topic, subscription);
}
return newPartitionGroupMetadataList;
}
+ private void deleteSubscription(String topicName, String subscription) {
+ try {
+ _pulsarAdminClient.topics().deleteSubscription(topicName, subscription);
Review Comment:
I couldn't find any doc around this and haven't got much response in pulsar slack. since I have to look at the pulsar code next, how about we re-visit this and leave the implementation in the PR as it is, @mathieudruart ?
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] mathieudruart commented on a diff in pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
mathieudruart commented on code in PR #9859:
URL: https://github.com/apache/pinot/pull/9859#discussion_r1036065677
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarStreamMetadataProvider.java:
##########
@@ -156,11 +174,21 @@ public List<PartitionGroupMetadata> computePartitionGroupMetadata(String clientI
LOGGER.warn("Error encountered while calculating pulsar partition group metadata: " + e.getMessage(), e);
} finally {
closeConsumer(consumer);
+ deleteSubscription(_topic, subscription);
}
return newPartitionGroupMetadataList;
}
+ private void deleteSubscription(String topicName, String subscription) {
+ try {
+ _pulsarAdminClient.topics().deleteSubscription(topicName, subscription);
Review Comment:
I haven't tested, but maybe a non-durable subscription doesn't need to be deleted? maybe it is automatically deleted once consumer is closed?
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #9859:
URL: https://github.com/apache/pinot/pull/9859#issuecomment-1326106297
# [Codecov](https://codecov.io/gh/apache/pinot/pull/9859?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#9859](https://codecov.io/gh/apache/pinot/pull/9859?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6a5aaa1) into [master](https://codecov.io/gh/apache/pinot/commit/214095425e3cd084e2cbb53dc6a52e65cf0d6e9d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2140954) will **decrease** coverage by `42.13%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #9859 +/- ##
=============================================
- Coverage 70.34% 28.21% -42.14%
+ Complexity 5014 53 -4961
=============================================
Files 1972 1960 -12
Lines 105692 105334 -358
Branches 15993 15950 -43
=============================================
- Hits 74350 29717 -44633
- Misses 26137 72694 +46557
+ Partials 5205 2923 -2282
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `25.20% <ø> (-0.01%)` | :arrow_down: |
| integration2 | `24.49% <ø> (-0.15%)` | :arrow_down: |
| unittests1 | `?` | |
| unittests2 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/9859?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...in/java/org/apache/pinot/spi/utils/BytesUtils.java](https://codecov.io/gh/apache/pinot/pull/9859/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQnl0ZXNVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/BaseRecording.java](https://codecov.io/gh/apache/pinot/pull/9859/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvQmFzZVJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/spi/trace/NoOpRecording.java](https://codecov.io/gh/apache/pinot/pull/9859/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdHJhY2UvTm9PcFJlY29yZGluZy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/table/FSTType.java](https://codecov.io/gh/apache/pinot/pull/9859/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL0ZTVFR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/config/user/RoleType.java](https://codecov.io/gh/apache/pinot/pull/9859/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3VzZXIvUm9sZVR5cGUuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/data/MetricFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/9859/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9NZXRyaWNGaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pinot/spi/stream/StreamMessage.java](https://codecov.io/gh/apache/pinot/pull/9859/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvc3RyZWFtL1N0cmVhbU1lc3NhZ2UuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...java/org/apache/pinot/common/tier/TierFactory.java](https://codecov.io/gh/apache/pinot/pull/9859/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdGllci9UaWVyRmFjdG9yeS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/spi/config/table/TableType.java](https://codecov.io/gh/apache/pinot/pull/9859/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvY29uZmlnL3RhYmxlL1RhYmxlVHlwZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pinot/spi/data/DimensionFieldSpec.java](https://codecov.io/gh/apache/pinot/pull/9859/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvZGF0YS9EaW1lbnNpb25GaWVsZFNwZWMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1360 more](https://codecov.io/gh/apache/pinot/pull/9859/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] navina commented on a diff in pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
navina commented on code in PR #9859:
URL: https://github.com/apache/pinot/pull/9859#discussion_r1032874773
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarStreamMetadataProvider.java:
##########
@@ -156,11 +174,21 @@ public List<PartitionGroupMetadata> computePartitionGroupMetadata(String clientI
LOGGER.warn("Error encountered while calculating pulsar partition group metadata: " + e.getMessage(), e);
} finally {
closeConsumer(consumer);
+ deleteSubscription(_topic, subscription);
}
return newPartitionGroupMetadataList;
}
+ private void deleteSubscription(String topicName, String subscription) {
+ try {
+ _pulsarAdminClient.topics().deleteSubscription(topicName, subscription);
Review Comment:
> I think we should create them non-durable, but maybe also delete them to clean the memory of the Pulsar brokers.
I wasn't aware of this mode. But what is the benefit of creating them non-durable, if we have to delete them anyway?
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] mathieudruart commented on a diff in pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
mathieudruart commented on code in PR #9859:
URL: https://github.com/apache/pinot/pull/9859#discussion_r1035195690
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarStreamMetadataProvider.java:
##########
@@ -156,11 +174,21 @@ public List<PartitionGroupMetadata> computePartitionGroupMetadata(String clientI
LOGGER.warn("Error encountered while calculating pulsar partition group metadata: " + e.getMessage(), e);
} finally {
closeConsumer(consumer);
+ deleteSubscription(_topic, subscription);
}
return newPartitionGroupMetadataList;
}
+ private void deleteSubscription(String topicName, String subscription) {
+ try {
+ _pulsarAdminClient.topics().deleteSubscription(topicName, subscription);
Review Comment:
@navina yes I suppose we can
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] KKcorps commented on a diff in pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #9859:
URL: https://github.com/apache/pinot/pull/9859#discussion_r1035941781
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarStreamMetadataProvider.java:
##########
@@ -156,11 +174,21 @@ public List<PartitionGroupMetadata> computePartitionGroupMetadata(String clientI
LOGGER.warn("Error encountered while calculating pulsar partition group metadata: " + e.getMessage(), e);
} finally {
closeConsumer(consumer);
+ deleteSubscription(_topic, subscription);
}
return newPartitionGroupMetadataList;
}
+ private void deleteSubscription(String topicName, String subscription) {
+ try {
+ _pulsarAdminClient.topics().deleteSubscription(topicName, subscription);
Review Comment:
IMO, we can make this change. This subscription is only used to fetch metadata so it is fine if its non durable.
https://pulsar.apache.org/docs/2.10.x/concepts-messaging/#subscription-modes
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] KKcorps commented on a diff in pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
KKcorps commented on code in PR #9859:
URL: https://github.com/apache/pinot/pull/9859#discussion_r1035942175
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarStreamMetadataProvider.java:
##########
@@ -156,11 +174,21 @@ public List<PartitionGroupMetadata> computePartitionGroupMetadata(String clientI
LOGGER.warn("Error encountered while calculating pulsar partition group metadata: " + e.getMessage(), e);
} finally {
closeConsumer(consumer);
+ deleteSubscription(_topic, subscription);
}
return newPartitionGroupMetadataList;
}
+ private void deleteSubscription(String topicName, String subscription) {
+ try {
+ _pulsarAdminClient.topics().deleteSubscription(topicName, subscription);
Review Comment:
Other than that, PR LGTM!
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] mathieudruart commented on a diff in pull request #9859: Use Pulsar AdminClient to delete unused subscriptions
Posted by GitBox <gi...@apache.org>.
mathieudruart commented on code in PR #9859:
URL: https://github.com/apache/pinot/pull/9859#discussion_r1031698031
##########
pinot-plugins/pinot-stream-ingestion/pinot-pulsar/src/main/java/org/apache/pinot/plugin/stream/pulsar/PulsarStreamMetadataProvider.java:
##########
@@ -156,11 +174,21 @@ public List<PartitionGroupMetadata> computePartitionGroupMetadata(String clientI
LOGGER.warn("Error encountered while calculating pulsar partition group metadata: " + e.getMessage(), e);
} finally {
closeConsumer(consumer);
+ deleteSubscription(_topic, subscription);
}
return newPartitionGroupMetadataList;
}
+ private void deleteSubscription(String topicName, String subscription) {
+ try {
+ _pulsarAdminClient.topics().deleteSubscription(topicName, subscription);
Review Comment:
I saw that it was possible to create lightweight subscriptions using non durable subscription mode :
Consumer<byte[]> consumer = pulsarClient.newConsumer()
.topic("my-topic")
.subscriptionName("my-sub")
.subscriptionMode(SubscriptionMode.NonDurable)
.subscribe();
in this case the subscription is still stored in the Pulsar broker memory (but no longer persisted with a durable cursor).
I think we should create them non-durable, but maybe also delete them to clean the memory of the Pulsar brokers.
--
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@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org