Reconcile terminology and method naming to 'used/unused segments'; Rename MetadataSegmentManager to MetadataSegmentsManager

[leventov]

Description

There are several related sets of changes:

Reconcile terminology and method naming to 'used/unused segments'

This naming stems from the used=0/1 column in the database table. Other names that used to be used in the codebase are

• "database segments"
• "database available segments"
• "available segments"
• "enabled/disabled segments"
• "delete/remove/kill segments", in the meaning of "mark segments as unused"

You can see how the third one, "available segments" has emerged, but the result is totally misleading and confusing with segments which are available aka served by some data nodes.

Don't use terms 'enable/disable data source'

"Enable data source" actually means "try to mark as 'used' all segments in the database that belong to a data source". "Disable data source" actually means "try to mark as unused all segments in the database that belong to a data source". Enable/disable terminology might make users think that Druid maintains a separate toggle with the notion of an individual segment's used flag, which is false. Disabling a data source and then enabling it again loses information about the subset of segments that may have been unused in a data source.

Similarly, uses terms 'Used/unused data source' instead of 'enabled/disabled data source' to differentiate between data sources to which at least one used segment belongs and to which none used segments belong.

Rename MetadataSegmentManager to SegmentsMetadataManager

To make less aliasing and confusion with SegmentsManager, a class used on Historical nodes. Also renamed residual occurrences in variable names of "DatabaseSegmentManager", which is the former name of MetadataSegmentManager.

Rename DruidCoordinatorHelper to CoordinatorDuty

Also, renamed the implementations:

• DruidCoordinatorBalancer -> BalanceSegments
• DruidCoordinatorSegmentCompactor -> CompactSegments
• DruidCoordinatorLogger -> EmitClusterStatsAndMetrics
• DruidCoordinatorSegmentKiller -> KillUnusedSegments
• DruidCoordinatorCleanupPendingSegments -> KillStalePendingSegments
• DruidCoordinatorSegmentInfoLoader -> LogUsedSegments
• DruidCoordinatorCleanupOvershadowed -> MarkAsUnusedOvershadowedSegments
• DruidCoordinatorCleanupUnneeded -> UnloadUnusedSegments
• DruidCoordinatorRuleRunner -> RunRules

Also, renamed CoordinatorIndexingServiceHelper annotation to CoordinatorIndexingServiceDuty.

Rename IndexerSQLMetadataStorageCoordinator's get* methods to retrieve*

Because they all access the metadata store, i. e. not cheap.

• getUsedSegmentsForInterval -> retrieveUsedSegmentsForInterval
• getAllUsedSegments -> retrieveAllUsedSegments
• getUsedSegmentsAndCreatedDates -> retrieveUsedSegmentsAndCreatedDates
• getUsedSegmentsForIntervals -> retrieveUsedSegmentsForIntervals
• getveUnusedSegmentsForInterval -> retrieveUnusedSegmentsForInterval
• getDataSourceMetadata -> retrieveDataSourceMetadata

As well as a related method UsedSegmentLister.getUsedSegmentsForIntervals -> retrieveUsedSegmentsForIntervals.

See also #8187 and #8188.

Rename SegmentsAndMetadata class to SegmentsAndCommitMetadata

This is done to increase the difference from SegmentsMetadataManager (former MetadataSegmentManager).

Rename several indexing service task-related classes

• KillTask -> KillUnusedSegmentsTask
• ClientQuery -> ClientTaskQuery
• ClientKillQuery -> ClientKillUnusedSegmentsTaskQuery
• ClientCompactQuery -> ClientCompactionTaskQuery

Refactoring of DruidCoordinator

Refactored DruidCoordinator, removed unnecessary CoordinatorRunnable hierarchy.

Future work

• Rename MetadataRuleManager to RulesMetadataManager to align with SegmentsMetadataManager.
• Actually make POST /druid/coordinator/v1/datasources/{dataSourceName}/segments/{segmentId} to mark as used only non-overshadowed segments?

---

Key files changed in this PR are datasource-view.tsx and DruidCoordinator.

[gianm]

Reconcile terminology and method naming to 'used/unused segments'

This looks related and somewhat conflicting with the terminology discussion on #7233, where initial confusion over the meaning of is_published led to a suggestion of creating a new term "active". What do you think about the suggestion on #7233 (comment)?

I think if we go with where 7233 is going, the term for "used segments" would be "published segments". Fwiw, one reason I like this term because it lends itself well to being used as verbs in the following way:

• A task would first push a segment to deep storage, and then publish it to the metadata store.
• When a segment is dropped it is unpublished from the metadata store by setting used = false.

Don't use terms 'enable/disable data source'

Yeah, I definitely also believe these terms are not great, and are confusing for the reasons you mention. Here the "publish" and "unpublish" terms are a bit awkward though. It doesn't sound right to say "publish all segments in a datasource" instead of "enable a datasource". "Mark used" or "activate" sounds better. I'm not sure how to reconcile this with the fact that I do think "publish" sounds good & intuitive as a verb in the case where a task is initially writing a segment record to the metadata store.

Maybe this is how:

• Tasks "publish" segments when they insert records into the metadata store with used = true (upon creation of the segment)
• Later on if we drop a segment, we do that by "marking it unused" or, potentially, "deactivating" it.
• If we re-enable a segment, we do that by "marking it used", or, potentially, "activating" it.

[leventov]

I think it makes sense to keep terminology between virtual Druid's SQL table sys.segments and metadata store (and therefore the remaining of the codebase) separate.

Can used be renamed? Or not, because it's a column in the database that needs to remain compatible?

[gianm]

I think it makes sense to keep terminology between virtual Druid's SQL table sys.segments and metadata store (and therefore the remaining of the codebase) separate.

I don't think we need to be dogmatic about it. But if possible, it benefits everyone (users & developers alike) to have consistent terminology across the entire system.

Can used be renamed? Or not, because it's a column in the database that needs to remain compatible?

Do you mean in the metadata store? I don't think it could be, since we don't have a way to automatically migrate already-existing metadata stores that use the name used. But we could decide we want to call the concept something else in general, and just keep a note that for legacy reasons, the metadata store uses the old name.

[leventov]

But if possible, it benefits everyone (users & developers alike) to have consistent terminology across the entire system.

I'm not sure that a column in sys.segments and the internal codebase concept (currently "used segments") are the same thing. Calling them the same may add more confusion and make navigation in the codebase harder than if the names are different. For example, if sys.segments concept is called "published" and somebody sees "published" in code, they know it's about the virtual SQL table, not the table in the metadata store.

[gianm]

I'm not sure that a column in sys.segments and the internal codebase concept (currently "used segments") are the same thing.

I think it's best if they are the same thing, since the point of system tables like sys.segments is to help cluster operators introspect into the state of the system using a familiar SQL style API. (So they don't have to go digging through metadata store, ZK, various Druid APIs to get a picture of what's going on.)

If it turns out that for some reason, they can't be the same thing, then it makes sense to use two different names. But if they could be the same thing then it's probably best to do that.

[leventov]

@gianm I didn't question that "published in sys.segments" and "used in metadata store" is the same thing, logically. I did question whether making this same logical thing called the same would actually make positive, rather than negative impact on the codebase. However, now I also think it's higher probability that it will have positive impact on the codebase. So I will support the rename.

It doesn't seem to me that this PR is conflicting with the rename. I think it's supportive to the rename because in this PR I did some nasty work at finding and sweeping old names and variants of naming that were looking completely unrelated. Now for the rename, somebody needs to make just two full-text searches in the codebase on Github (I did many more when preparing this PR):

• "used segment"
• "unused segment"

Then visit all result files that look relevant manually and rename "used/unused segments" to "publish/published/unpublished". (Note that this search needs to be performed after this PR is merged, because Github indexes only master codebase for full-text search.)

For this reason, I tag this PR Development Blocker for #7233.

Maybe this is how:

• Tasks "publish" segments when they insert records into the metadata store with used = true (upon creation of the segment)
• Later on if we drop a segment, we do that by "marking it unused" or, potentially, "deactivating" it.
• If we re-enable a segment, we do that by "marking it used", or, potentially, "activating" it.

Oh no, I think we must go all-in with publish/unpublish/published/unpublished terminology. So we should call these actions "mark (as) published", however awkward it sounds, and "unpublish" (I think "unpublish all segments in a data source" sounds fine), or, alternatively, "mark (as) unpublished".

[leventov]

For reviewers: in the latest commits I've made some more clarification renames:

• KillTask -> KillUnusedSegmentsTask
• TaskMonitor.killTask() -> cancelTask() (see also related Rename Overlord task "shutdown" to "cancel" #7361)
• IndexingServiceClient.killTask() -> cancelTask()
• IndexingServiceClient.killSegments() -> killUnusedSegments()
• ClientQuery -> ClientTaskQuery
• ClientKillQuery -> ClientKillUnusedSegmentsTaskQuery
• ClientCompactQuery -> ClientCompactionTaskQuery
• ClientCompactQueryTuningConfig -> ClientCompactionTaskQueryTuningConfig

CoordinatorDynamicConfig's properties:

• millisToWaitBeforeDeleting -> millisLagSinceCoordinatorBecomesLeaderBeforeCanMarkAsUnusedOvershadowedSegments
• killAllDataSources -> killUnusedSegmentsInAllDataSources
• specificDataSourcesToKill -> specificDataSourcesToKillUnusedSegmentsIn
• protectedPendingSegmentDatasources -> dataSourcesToNotKillStalePendingSegmentsIn

I kept the old names of JSON properties because aliases (for backward compatibility) are only supported in Jackson 2.9, see #7152.

DruidCoordinatorHelpers:

• DruidCoordinatorCleanupPendingSegments -> DruidCoordinatorKillStalePendingSegments
• DruidCoordinatorCleanupUnusedSegments -> DruidCoordinatorUnloadUnusedSegments
• DruidCoordinatorCleanupOvershadowed -> DruidCoordinatorMarkAsUnusedOvershadowedSegments
• DruidCoordinatorSegmentKiller -> DruidCoordinatorUnusedSegmentsKiller

(Next steps will be: remove "DruidCoordinator" prefixes and make all class names noun phrases or verb phrases, but not a mix.)

[egor-ryashin]

SegmentsStorage ? ...Segments make me think about a factory class, while this one also makes update and delete operations.

[egor-ryashin]

Also, it's more like segmentsMetadata we work with here, not ...Segments directly, meaning, can we name it SegmentsMetadataStorage ?

[leventov]

Could you explain the factory association? I don't see a link.

To me, "Storage" doesn't contribute to meaning, as well as "Manager" which I removed.

SegmentsSqlMetadata may be a better name than SqlMetadataSegments though.

[leventov]

Renamed the interface to SegmentsMetadata and the implementation class to SqlSegmentsMetadata respectively.

[egor-ryashin]

I would like to see classes that contain DB API calls - to identify themselves in naming the consistent way across the project. So do you propose to start replacing Manager to Sql or it will be a single stray class like that?

[leventov]

I don't replace Manager with Sql - SQL was already in the name. I just removed Manager.

[egor-ryashin]

Got it. But what about I would like to see classes that contain DB API calls - to identify themselves in naming the consistent way across the project.

[leventov]

I think Sql is that identifier.

[egor-ryashin]

I wonder if All is necessary?

[leventov]

Do you think is unnecessary?

[egor-ryashin]

All makes sense in a specific use case, ie If there are two methods like:

getAll
getRandomSample

otherwise All is just redundant.

[leventov]

This rename is already done and merged in #7653.

[egor-ryashin]

Hmm, it's confusing to see it here then, I spent time reading through those changes.

[egor-ryashin]

I wonder if All is necessary?

[egor-ryashin]

I wonder if As is necessary?

[egor-ryashin]

This question is missed, I guess.

[leventov]

This rename is already done and merged in #7653. This PR doesn't rename methods in MetadataSegmentManager anymore, it only renames the class itself.

[egor-ryashin]

For me it's much easier/naturally to find a description of the method/variable then to read so long name without spaces between words.

[leventov]

Do you think it's easier for everyone, for example, people who are not Druid developers themselves?

[jon-wei]

I also think that name is too long, suggest initialDropOvershadowedWaitMillis instead, with additional clarification in docs

[jon-wei]

Or markOvershadowedAsUnusedInitialWaitMillis

[leventov]

I don't like the loss of meaning or even confusion in the "initial" adjective. Also, other configuration parameter names in this class don't drop the "Segments" part, except for replicantLifetime and replicationThrottleLimit which are bad names anyway. I also wanted to keep the "MarkAsUnused" part together, because it appears in different places in the code. What about leadingTimeMillisBeforeCanMarkAsUnusedOvershadowedSegments?

[egor-ryashin]

Whoa, it takes me a deal of concentration just to check those two variables represent the same property. Now I know when the bigger is not better regarding the variable naming.

[leventov]

Do you have some better names in mind?

[egor-ryashin]

millisLagBeforeMarkingUnusedOvershadowed

[leventov]

This property is already renamed to leadingTimeMillisBeforeCanMarkAsUnusedOvershadowedSegments in #7653.

[egor-ryashin]

"capacity" implies the maximum limit
with max free task slots ?

[leventov]

I don't know.

[egor-ryashin]

Let's do Tasks are assigned to the middleManager with the most free slots at the time

[egor-ryashin]

Also, let's write Middle Manager

[leventov]

Applied the first suggestion. I didn't replace the term "middleManager" (one word) with "Middle Manager" because the first version is used throughout the document in many places. I'm also not sure that the separate writing is better for searching in the docs. If you feel that the replacement would be an improvement, please open an issue.

[egor-ryashin]

Total size of used segments ?

[leventov]

I don't understand what do you mean by this question.

[egor-ryashin]

Let's do Sum of sizes of all used segments -> Total size of used segments

[leventov]

Changed

[egor-ryashin]

allTimelineEntries makes me think there's another collection with a subset of "all", but it isn't.

[leventov]

It follows the naming of "allTimelineEntries". I think the logic behind this naming is that it includes overshadowed objects. I've extended the Javadoc of iterateAllObjects() to make it clearer.

[leventov]

@pjain1 could you please take another look? I didn't test UI yet though because I have problems preparing a docker image though

[lgtm-com]

This pull request introduces 1 alert when merging 6da0b9d into 441515c - view on LGTM.com

new alerts:

• 1 for Useless comparison test

[jihoonson]

@leventov, I apologize for the very delayed review and thank you for your patience. Also thank you for adding a bunch of missing javadocs and comments. I left a couple of comments.

It doesn't seem to me that this PR is conflicting with the rename. I think it's supportive to the rename because in this PR I did some nasty work at finding and sweeping old names and variants of naming that were looking completely unrelated. Now for the rename, somebody needs to make just two full-text searches in the codebase on Github (I did many more when preparing this PR):

"used segment"
"unused segment"

Then visit all result files that look relevant manually and rename "used/unused segments" to "publish/published/unpublished". (Note that this search needs to be performed after this PR is merged, because Github indexes only master codebase for full-text search.)

I'm wondering you're planning to rename "used/unused segments" to "published/unpublished segments" in the near future. If not, I think this PR should change those names properly at least for the UI. If you do, I promise to review your follow-up PR as soon as possible.

[jihoonson]

Would you add a javadoc saying when it can be null? Perhaps it would be

  /**
   * Returns the value of the valueColumn when there is only one row matched to the given key.
   * This method returns null if there is no such row and throws an error if there are more than one rows.
   */

[leventov]

Thanks for suggestion, applied

[jihoonson]

Hmm, the method name doesn't look valid anymore. Maybe lazyCollectionOfSegmentIds?
Also why not having Collection<SegmentId> as the return type instead of Object?

[leventov]

The purpose is to make this method unusable for anything except logging. To obtain a collection of SegmentIds, people should usually use Stream API to create a non-lazy collection.

[jihoonson]

Hmm, I intentionally used the singular form here to say that there is only one visibleGroup in any case. Perhaps the code should be refactored to be more clear.

[leventov]

I renamed to visibleGroupPerRange, does it make more sense? Also added a comment as per #8788

[jihoonson]

It sounds good to me. Thanks for adding a comment.

[jihoonson]

Before any unassigned segments are serviced by Historical processes, the Historical processes for each tier are first
sorted in terms of capacity, with least capacity servers having the highest priority.

It seems like this statement was written in 2013.. I don't think this is true anymore. Maybe better to mention druid.coordinator.balancer.strategy and link https://github.com/apache/druid/blob/master/docs/configuration/index.md#coordinator-operation.

[leventov]

I think coordinator documentation should better be updated at once, There is #7201 tracking this

[jihoonson]

Sounds good to me.

[jihoonson]

Suggest some nodes -> historical processes.

[leventov]

changed to Historical nodes because this term is also used in the section twice.

[jihoonson]

I think it's more clear to say "The returned collection may include overshadowed segments and their created_dates, as if {@link Segments#INCLUDING_OVERSHADOWED} was passed. It's the responsibility of the caller to filter out overshadowed ones if needed`.

[leventov]

Thanks, changed.

[jihoonson]

typo: interva -> interval

[leventov]

Thanks, fixed

[jihoonson]

This method doesn't seem to check the created_date field.

[leventov]

Why? Looks it does:

druid/server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java

Lines 894 to 910 in 6da0b9d

	@Override
	public int deletePendingSegmentsCreatedInInterval(String dataSource, Interval deleteInterval)
	{
	return connector.getDBI().inTransaction(
	(handle, status) -> handle
	.createStatement(
	StringUtils.format(
	"DELETE FROM %s WHERE datasource = :dataSource AND created_date >= :start AND created_date < :end",
	dbTables.getPendingSegmentsTable()
	)
	)
	.bind("dataSource", dataSource)
	.bind("start", deleteInterval.getStart().toString())
	.bind("end", deleteInterval.getEnd().toString())
	.execute()
	);
	}

[jihoonson]

This javadoc is for a different method: int deletePendingSegments(String dataSource);

[leventov]

I see now, fixed. Thanks.

[jihoonson]

I think it's more popular to put annotations before the line where the method access modifier is (just like @Override). Is this intended?

[leventov]

I prefer putting @Nullable before the type because semantically it characterizes the returned type, not the method (there are also modifiers and other annotations in between).

[jihoonson]

I think duty is more clear to me than helper.

[lgtm-com]

This pull request introduces 1 alert when merging 10587c1 into d541cbe - view on LGTM.com

new alerts:

• 1 for Useless comparison test

[leventov]

@jihoonson thank you very much for taking time to review this PR.

I'm wondering you're planning to rename "used/unused segments" to "published/unpublished segments" in the near future. If not, I think this PR should change those names properly at least for the UI. If you do, I promise to review your follow-up PR as soon as possible.

I don't plan to make a follow-up rename. I don't want to make partial rename in the UI because it will create an inconsistency between UI and official documentation. Then, if docs are changed, there is an inconsistency between docs and code. And so on.

So I think there should be a one-time rename. In the part of my comment which you cited, I explained why I think that this PR in its current state is at least not a move in the wrong direction, in terms of rename, because it will greatly simplify the future rename, whoever decided to do it.

[jihoonson]

I don't plan to make a follow-up rename. I don't want to make partial rename in the UI because it will create an inconsistency between UI and official documentation. Then, if docs are changed, there is an inconsistency between docs and code. And so on.

So I think there should be a one-time rename. In the part of my comment which you cited, I explained why I think that this PR in its current state is at least not a move in the wrong direction, in terms of rename, because it will greatly simplify the future rename, whoever decided to do it.

@leventov thanks, it makes sense to me.

[vogievetsky]

This is a great rename, it is good to have the UI aligned with the docs. Could you please make sure that we treat "datasource" as one word I have been consistently using "datasource" for the Druid table thing and "data source" for the source of data like S3 or HDFS (in the data loader).

[vogievetsky]

So this should be datasourceToMarkSegmentsByIntervalIn

[leventov]

Renamed

[vogievetsky]

This is way too long to be a button. Maybe just call it "Mark as unused" and make sure to specify what is going on in the text of the dialog (which you are mostly doing already)

[leventov]

I shortened confirm button texts but just removed "in data source" parts, IMO even shorter names are too ambiguous.

[lgtm-com]

This pull request introduces 1 alert when merging 2ad6dd5 into 83ddc8d - view on LGTM.com

new alerts:

• 1 for Useless comparison test

[jihoonson]

The latest changes LGTM. @vogievetsky do you have more comments? @leventov sorry to say, but would you mind fixing the conflict one more time?

[vogievetsky]

Web console changes LGTM 👍

[leventov]

An interesting lucky occasion: the conflict was in the specific file and very close to the line where I actually inserted a bug, which was not caught by tests. (Raised #9257)

[leventov]

@jihoonson @vogievetsky thanks for reviews!

[lgtm-com]

This pull request introduces 1 alert when merging 6745f4a into 19b427e - view on LGTM.com

new alerts:

• 1 for Useless comparison test

[jihoonson]

The line where LGTM complains about is to verify two static variables have the same value which should have been the same variable but stored separately because of the module dependency. So, the LGTM failure doesn't look legit in this case. I'm not sure whether we can skip the LGTM inspection for some particular lines. I'm merging this PR since it has been open for too long time. We can address this issue later in a follow-up PR.