realtime cleanup and renaming by clintropolis · Pull Request #16652 · apache/druid

clintropolis · 2024-06-25T11:02:34Z

changes:

FireHydrant is now PartialSegment. This name much more clearly describes what this class does, and with all the other fireman terminology removed it didn't even fit a theme anymore.
Sink is now AppendableSegment. This name also much more clearly describes what this class does, and is composed of PartialSegments per the previous FireHydrant rename.
Additionally, SinkQuerySegmentWalker -> AppendableSegmentQuerySegmentWalker, and SinkQueryRunner -> AppendableSegmentQueryRunner
Remove Firehose, IngestSegmentFirehose was only used by Hadoop indexing DruidRecordReader, moved to internal class of DruidRecordReader as SegmentReader
Remove FirehoseFactory and remaining implementations, after Remove index_realtime and index_realtime_appenderator tasks #16602 they were no longer used
Moved things from org.apache.druid.segment.realtime.sink and org.apache.druid.segment.realtime.firehose up to org.apache.druid.segment.realtime.

Discussion: should we move metric ingest/sink/count to ingest/appendableSegment/count or something similar? That is kind of disruptive since there is like a continuity of measurement issue which is kind of sad. We could also always double emit it, but also sad in its own way.

Release note

todo

This PR has:

been self-reviewed.
added documentation for new or modified features or behaviors.
a release note entry in the PR description.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added or updated version, license, or notice information in licenses.yaml
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
added integration tests.
been tested in a test Druid cluster.

changes: * `FireHydrant` is now `PartialSegment`. This name much more clearly describes what this class does, and with all the other fireman terminology removed it didn't even fit a theme anymore. * `Sink` is now `AppendableSegment`. This name also much more clearly describes what this class does, and is composed of `PartialSegments` per the previous `FireHydrant` rename. * Additionally, `SinkQuerySegmentWalker` -> `AppendableSegmentQuerySegmentWalker`, and `SinkQueryRunner` -> `AppendableSegmentQueryRunner` * Remove `Firehose`, `IngestSegmentFirehose` was only used by Hadoop indexing `DruidRecordReader`, moved to internal class of `DruidRecordReader` as `SegmentReader` * Remove `FirehoseFactory` and remaining implementations, after apache#16602 they were no longer used * Moved things from `org.apache.druid.segment.realtime.sink` and `org.apache.druid.segment.realtime.firehose` up to `org.apache.druid.segment.realtime`.

+    expectedException.expectMessage("segment.close() is called somewhere outside PartialSegment.swapSegment()");
+    partialSegment.getPartialSegment().close();
+    Assert.assertEquals(0, partialSegment.getPartialSegment().getNumReferences());
+    ReferenceCountingSegment segment = partialSegment.getIncrementedSegment();


-      final int hydrantNumber = Integer.parseInt(hydrantDir.getName());
+    List<PartialSegment> partialSegments = new ArrayList<>();
+    for (File partialSegmentDir : appendableSegmentFiles) {
+      final int partialSegmentNumber = Integer.parseInt(partialSegmentDir.getName());


-          final int hydrantNumber = Integer.parseInt(hydrantDir.getName());
+        List<PartialSegment> partialSegments = new ArrayList<>();
+        for (File partialSegmentDir : appendableSegmentFiles) {
+          final int partialSegmentNumber = Integer.parseInt(partialSegmentDir.getName());


-          final int hydrantNumber = Integer.parseInt(hydrantDir.getName());
+        List<PartialSegment> partialSegments = new ArrayList<>();
+        for (File partialSegmentDir : appendableSegmentFiles) {
+          final int partialSegmentNumber = Integer.parseInt(partialSegmentDir.getName());


  }

-  private int calculateSinkMemoryInUsed(Sink sink)
+  private int calculateAppendableSegmentMemoryInUsed(AppendableSegment appendableSegment)


kfaraz · 2024-06-25T13:42:37Z

Discussion: should we move metric ingest/sink/count to ingest/appendableSegment/count or something similar? That is kind of disruptive since there is like a continuity of measurement issue which is kind of sad. We could also always double emit it, but also sad in its own way.

I think it is okay to deprecate the older metric but keep emitting it along with the new one for a couple of releases.
We are already emitting a good number of metrics, one more is not going to hurt.
But at least this way we would be able to eventually get rid of the old one.

Also, @clintropolis , what do you think about calling it RealtimeSegment instead of AppendableSegment?
"Realtime segment" is a term that Druid community is already familiar with.
"Appendable segment" seems slightly ambiguous as it could be misinterpreted by some to mean a segment to which more data can be added even after it has been committed.

clintropolis · 2024-06-26T08:22:27Z

Also, @clintropolis , what do you think about calling it RealtimeSegment instead of AppendableSegment?
"Realtime segment" is a term that Druid community is already familiar with.
"Appendable segment" seems slightly ambiguous as it could be misinterpreted by some to mean a segment to which more data can be added even after it has been committed.

My reasoning for AppendableSegment was because they are built with an Appenderator. Also "realtime" isn't really appropriate because they are also created during batch processing. I would be ok changing the metric name to something else, but I think the current name is more appropriate in code at least. I am open to other suggestions though, I considered 'mutable' segment but that is really only part of the AppendableSegment which is composed of one mutable PartialSegment and several other immutable intermediary persisted PartialSegment (using the new naming).

kfaraz · 2024-06-26T09:23:43Z

Also "realtime" isn't really appropriate because they are also created during batch processing.

Yeah, I wondered if this was the case, didn't see that BatchAppenderator itself is using sinks.
If I am not mistaken, a sink should always corresponding to a pending segment i.e. an entry in the pendingSegments metadata table. So maybe PendingSegment could be a better name?

clintropolis · 2024-06-26T10:15:01Z

If I am not mistaken, a sink should always corresponding to a pending segment i.e. an entry in the pendingSegments metadata table. So maybe PendingSegment could be a better name?

PendingSegment doesn't seem quite right to me because it seems like an odd name for the role that this thing plays in code. I think my problem here is the word 'pending' which seems like something you wouldn't want to be using in stuff like query processing because it sounds like something that isn't 'official' yet - which makes sense for how it is used for task locks, but less so for the internal code stuff imo.

What don't you like about AppendableSegment as an internal code construct name? Should we rename Appenderator too? I don't think it would be useful for the docs to talk about them as this and agree it might be confusing, but I don't think the docs widely called them Sink either since this is just internal naming stuff.

kfaraz · 2024-06-26T12:08:01Z

I think my problem here is the word 'pending' which seems like something you wouldn't want to be using in stuff like query processing because it sounds like something that isn't 'official' yet

Agreed. I have myself been on the fence about the 'pending' part too as it is certainly confusing. But then, I am not sure what a better alternative would have been either (maybe open, appending or appendable itself 😛 ).

In that regard, I do like AppendableSegment as it does clarify that this is a segment to which we can currently append more rows. I just wanted to be sure before we introduced a new(-ish) term in the code.

You seem to have given it enough thought and I do agree with the arguments against the other options, so let's go ahead with AppendableSegment!

I don't think we need to rename Appenderator as it works fine for its role.

clintropolis · 2024-07-19T09:42:22Z

closing in favor of #16758

clintropolis added Refactoring Area - Ingestion labels Jun 25, 2024

github-actions Bot added Area - Documentation Area - Batch Ingestion Area - Querying Area - Streaming Ingestion Kubernetes Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Jun 25, 2024

github-advanced-security AI found potential problems Jun 25, 2024

View reviewed changes

clintropolis added 2 commits June 25, 2024 04:31

missing header

270c7e7

Merge remote-tracking branch 'upstream/master' into names-are-important

8dac4d3

fixes

6662a8c

fixes

4bf4bb2

clintropolis mentioned this pull request Jul 19, 2024

remove Firehose and FirehoseFactory #16758

Merged

clintropolis closed this Jul 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

realtime cleanup and renaming#16652

realtime cleanup and renaming#16652
clintropolis wants to merge 5 commits intoapache:masterfrom
clintropolis:names-are-important

clintropolis commented Jun 25, 2024

Uh oh!

Check notice

Check notice

Check notice

Check notice

Check notice

kfaraz commented Jun 25, 2024

Uh oh!

clintropolis commented Jun 26, 2024

Uh oh!

kfaraz commented Jun 26, 2024

Uh oh!

clintropolis commented Jun 26, 2024 •

edited

Loading

Uh oh!

kfaraz commented Jun 26, 2024

Uh oh!

clintropolis commented Jul 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

clintropolis commented Jun 25, 2024

Release note

Uh oh!

Check notice

Check notice

Check notice

Check notice

Check notice

kfaraz commented Jun 25, 2024

Uh oh!

clintropolis commented Jun 26, 2024

Uh oh!

kfaraz commented Jun 26, 2024

Uh oh!

clintropolis commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfaraz commented Jun 26, 2024

Uh oh!

clintropolis commented Jul 19, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

clintropolis commented Jun 26, 2024 •

edited

Loading