Skip to content

remove deprecated standalone realtime node#7915

Merged
clintropolis merged 5 commits intoapache:masterfrom
clintropolis:trim-the-fat
Jul 3, 2019
Merged

remove deprecated standalone realtime node#7915
clintropolis merged 5 commits intoapache:masterfrom
clintropolis:trim-the-fat

Conversation

@clintropolis
Copy link
Copy Markdown
Member

@clintropolis clintropolis commented Jun 18, 2019 •
edited
Loading

This removes CliRealtime, RealtimeManager, etc. This has been deprecated for as long as I can remember, any reason to keep it around? I think I got everything directly related to just this stuff, anything else that can be removed?

closes #443, closes #737, closes #888, closes #1202, closes #1851, closes #1933, closes #2057, closes #2283, closes #2317, closes #2339, closes #2676, closes #3015, closes #3023, closes #3036, closes #4602, closes #4695

@vogievetsky
Copy link
Copy Markdown
Contributor

vogievetsky commented Jun 18, 2019

Do this will remove realtime nodes completely? Could we close all the super old issues that are about bugs in realtime nodes?

@clintropolis
Copy link
Copy Markdown
Member Author

clintropolis commented Jun 18, 2019

Do this will remove realtime nodes completely? Could we close all the super old issues that are about bugs in realtime nodes?

Yes, and ... hopefully? I've not looked too hard into what relevant issues could potentially be closed, will try to hunt them down and reference here.

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Jun 18, 2019

+2

@clintropolis
Copy link
Copy Markdown
Member Author

clintropolis commented Jun 18, 2019 •
edited
Loading

candidates to close from a quick search:
#443
#737
#888
#1202
#1851
#1933
#2057
#2283
#2317
#2339
#2676
#3015
#3023
#3036
#4602
#4695

Will keep hunting to see if I can find additional related issues

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Jun 18, 2019

@clintropolis can you fix merge conflicts?

@vogievetsky
Copy link
Copy Markdown
Contributor

vogievetsky commented Jun 19, 2019

Purely from an external standpoint - considering overall project simplicity: 👍

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Jun 19, 2019

I think it is about time to nix it. However, please don't break the doc links. Instead, have them point to pages that say that realtime nodes have been removed.

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Jun 23, 2019

I started a dev list thread calling attention to this PR and also mentioning some additional things that IMO, make sense to remove: https://lists.apache.org/thread.html/7b5ed4665ef30e325f99e02b91e86a282b832c64449535487c747e99@%3Cdev.druid.apache.org%3E

This includes:

  • IrcFirehoseFactory
  • KafkaEightFirehoseFactory
  • RabbitMQFirehoseFactory
  • RocketMQFirehoseFactory
  • TwitterSpritzerFirehoseFactory
  • The FirehoseFactoryV2 and FirehoseV2 interfaces, which are only used by Realtime nodes, and all implementations of them

@leventov leventov requested a review from egor-ryashin June 25, 2019 16:31
egor-ryashin
egor-ryashin previously requested changes Jun 25, 2019
View reviewed changes
Comment thread docs/content/ingestion/standalone-realtime.md Outdated
org.apache.druid.cli.Main server realtime
```

This model of stream pull ingestion was deprecated and removed completely in Druid 0.16.0. Please consider using the
Copy link
Copy Markdown
Contributor

@egor-ryashin egor-ryashin Jun 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Could we provide a description here why it was deprecated?

Copy link
Copy Markdown
Contributor

@fjy fjy Jun 25, 2019

Choose a reason for hiding this comment

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

@egor-ryashin real-time nodes have been deprecated for years. It has been clearly stated for some time now that they will be removed. They suffer from numerous problems and we have not recommended that anyone use them for a long time now.

Copy link
Copy Markdown
Contributor

@egor-ryashin egor-ryashin Jun 26, 2019

Choose a reason for hiding this comment

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

They suffer from numerous problems
@fjy I actually would like that we write here what type of problems.

Copy link
Copy Markdown
Contributor

@gianm gianm Jun 27, 2019

Choose a reason for hiding this comment

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

I think probably the biggest reason is they are really finicky (every realtime node needs a unique configuration) and also they have some design issues that make it not possible to achieve exactly once ingestion (whereas the kafka/kinesis supervised ingestion methods can do that)

Copy link
Copy Markdown
Contributor

@gianm gianm Jun 27, 2019

Choose a reason for hiding this comment

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

Also the windowPeriod thing

Copy link
Copy Markdown
Member Author

@clintropolis clintropolis Jun 27, 2019

Choose a reason for hiding this comment

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

Thanks for the suggestion @egor-ryashin, this seems reasonable to add to me. I'll try to incorporate some of what @gianm said into the details on this page when I fix conflicts on this branch.

@vogievetsky
Copy link
Copy Markdown
Contributor

vogievetsky commented Jun 25, 2019

@gianm are you thinking the defunct firehoses should be removed as part of this PR or their own PR?

@egor-ryashin egor-ryashin dismissed their stale review June 27, 2019 17:18

Not a showstopper

@vogievetsky
Copy link
Copy Markdown
Contributor

vogievetsky commented Jun 29, 2019

Doc updates look great @clintropolis , thank you!

gianm
gianm approved these changes Jun 30, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

LGTM

jihoonson
jihoonson approved these changes Jul 1, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@jihoonson jihoonson left a comment

Choose a reason for hiding this comment

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

Thanks @clintropolis. I love this PR.

@clintropolis
Copy link
Copy Markdown
Member Author

clintropolis commented Jul 3, 2019

I think enough +1's between here and dev thread, going to merge

@clintropolis clintropolis merged commit f728337 into apache:master Jul 3, 2019
@clintropolis clintropolis deleted the trim-the-fat branch July 3, 2019 01:12
@clintropolis clintropolis mentioned this pull request Jul 3, 2019
Merged
2 tasks
@gianm gianm mentioned this pull request Jul 5, 2019
Closed
@clintropolis clintropolis added this to the 0.16.0 milestone Aug 8, 2019
@clintropolis clintropolis mentioned this pull request Aug 21, 2019
Closed
@clintropolis clintropolis mentioned this pull request Sep 24, 2021
Merged
9 tasks
@clintropolis clintropolis mentioned this pull request Oct 26, 2023
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment