You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/06/02 21:09:05 UTC
[GitHub] [druid] capistrant opened a new pull request, #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
capistrant opened a new pull request, #12599:
URL: https://github.com/apache/druid/pull/12599
<!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
<!-- Please read the doc for contribution (https://github.com/apache/druid/blob/master/CONTRIBUTING.md) before making this PR. Also, once you open a PR, please _avoid using force pushes and rebasing_ since these make it difficult for reviewers to see what you've changed in response to their reviews. See [the 'If your pull request shows conflicts with master' section](https://github.com/apache/druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master) for more details. -->
This PR addresses part of the proposal in #12526
<!-- Replace XXXX with the id of the issue fixed in this PR. Remove this section if there is no corresponding issue. Don't reference the issue in the title of this pull-request. -->
<!-- If you are a committer, follow the PR action item checklist for committers:
https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
### Description
<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
<!-- If there are several relatively logically separate changes in this PR, create a mini-section for each of them. For example: -->
#### Made an incompatible change to druid_segments table
Added a new column, `last_used VARCHAR(255)` to the druid_segments table. When creating the druid_segments table from scratch, this column is not nullable. However, to more easily facilitate an upgrade to a version of Druid with this code, the migration steps to ALTER druid_segments allow the column to have null values. Perhaps we can document an optional post upgrade step to ALTER the table to not allow nulls that is contingent upon completing the existing optional post-upgrade step to populate last_used for already unused rows.
This column is a UTC date string corresponding to the last time that the `used` column was modified.
#### Added a configuration to the `druid.coordinator.kill.` family, `druid.coordinator.kill.bufferPeriod`
`druid.coordinator.kill.bufferPeriod` is a Duration that defines the amount of time that a segment must have been unused for before KillUnusedSegments will potentially kill it. For example, with the default `PT24H`. If I mark a segment X unused at `2022-06-01T00:05:00.000Z`. By rule, this segment cannot be killed until at or after `2022-06-02T00:05:00.000Z`.
The most prominent use of this new configuration is to prevent deletion of data from Druid by mistake. It can be thought of like the trash folder in HDFS. Marking segments unused is the `rm`. Without trash, the data is just gone. With trash, we can recover. The period is the amount of time before pending trash sent away for good.
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
#### Alternative Design
Alternatively, I could have embedded last_used within the payload. The functionality at the end of the day would be the same, but I have listed some pros and cons below.
Pros
* No table schema change needed to upgrade Druid, making life easier for an operator.
Cons
* If you were adding this feature in the initial pre-release development of Druid, you'd probably steer away from accumulating too much in the payload column and instead create native columns in the database table
* embedding it in this column is almost hiding it. Or at least making it harder for others to discover and understand.
* You need to pull the payload back in the query for unused segments. Increased db I/O, network, druid memory and compute demands
* You need to process the payloads in Druid code instead of simply filtering on the column value if it were native to the metastore table
#### Upgrade Plan
Requiring a database table alteration is a breaking change. It introduces an extra requirement for an operator to upgrade from an older version of Druid. I have done my best to limit the complexity of upgrading and provide as much assistance as possible to the Druid operators out there.
##### No work necessary case
There is a case where there is no extra work necessary for the Druid operator to upgrade. If the operator has used the default of `true` for `druid.metadata.storage.connector.createTables`, and their metastore user has DDL privs, the table will automatically be altered at startup if the `last_used` column is missing from the druid_segments table.
The coordinator and overlord will startup just fine. And all future changes to the used column (plus new segment creation) will populate `last_used`. For existing segments who match `used = true`, the value of `last_used` will be `null`. I have coded up the logic for finding segments to kill in a way that ignores the bufferPeriod for these segments with `null` column values. This means that `KillUnusedSegments` will use the same logic as pre-upgrade when looking for killable segments in the metadata store.
##### Some work necessary case
If the operator has either:
* set `druid.metadata.storage.connector.createTables` to false
* not given their metastore user DDL privs
They will need to execute an ALTER statement themselves before the upgrade. To make things easy for them, I have created a new Druid cli tool called UpdateTables that can perform this for them by executing the same alter table code path as the no work necessary case described above. This tool, as well as the actual ALTER command - in case the operator wants to do it themselves - is documented in a new upgrade-prep.md resource.
##### Optional post upgrade action
If the operator is interested in populating all of their legacy unused segments with a last_used date string following the upgrade, I have included another action in the new UpdateTables cli tool that will do so. Again, I have documented the tool and provided the actual UPDATE statement that they could use manually. This step is completely optional and only needed if the operator desires that their legacy unused segments come under the purview of the new bufferPeriod config post-upgrade.
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
<hr>
##### Key changed/added classes in this PR
* MetadataStorageConnector - added 2 methods to interface
* IndexerSQLMetadataStorageCoordinator - create new segments with last_used column populated
* SQLMetadataConnector - code for altering and validating the druid_segments table
* SQLMetadataSegmentPublisher - publish segments with last_used column populated
* SegmentsMetadataManager - update signature of interface method
* SqlSegmentsMetadataManager - manage and use last_used column properly
* SqlSegmentsMetadataQuery.java - manage last_used properly
* DruidCoordinatorConfig - new config
* Main - new cli tool
* UpdateTables - new cli tool
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [ ] been self-reviewed.
- [ ] added documentation for new or modified features or behaviors.
- [ ] 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](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] 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](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by GitBox <gi...@apache.org>.
capistrant commented on PR #12599:
URL: https://github.com/apache/druid/pull/12599#issuecomment-1224898633
Thanks for the review and thoughts on changes @gianm ... I decided to go forward with the suggestion for background updates for unused segments with NULL value for new column. I wasn't completely sure on how implementation of this should look so some feedback on that code in SegmentsMetadataManager would be much appreciated. As of now thing "work" in my local cluster for testing, but I'm not sure it is the cleanest solution out there
My maven profile on my personal computer seems a bit messed up right now so I pushed despite not being able to check all the unit tests locally at this time. I'm working on fixing that tomorrow so I can ID any issues I may have introduced.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by GitBox <gi...@apache.org>.
capistrant commented on PR #12599:
URL: https://github.com/apache/druid/pull/12599#issuecomment-1349566343
> @capistrant - are you still working on this PR? Asking since this is a really useful feature. And after this, we can enable the auto-killing of unused segments.
@abhishekagarwal87 yes I am still very much interested in getting these merged upstream. I have continued to try and keep it up to date with master. I see that I have some recent conflicts that I have to resolve. I will try to fit that in this week
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by GitBox <gi...@apache.org>.
gianm commented on PR #12599:
URL: https://github.com/apache/druid/pull/12599#issuecomment-1154584701
As to the main change: what do you think of calling the new column `last_updated`? IMO, that's clearer, since `last_used` sounds like it might be the last time the segment was loaded or queried or something like that.
This change would be very welcome. Measuring time-to-kill from the last update time makes more sense than measuring it from the data interval.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty (druid)
Posted by "jon-wei (via GitHub)" <gi...@apache.org>.
jon-wei commented on PR #12599:
URL: https://github.com/apache/druid/pull/12599#issuecomment-1677882910
@capistrant Thanks for the update, taking a look at this today
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty (druid)
Posted by "jon-wei (via GitHub)" <gi...@apache.org>.
jon-wei commented on PR #12599:
URL: https://github.com/apache/druid/pull/12599#issuecomment-1648815618
@capistrant - Checking again to see if you're interested in picking this up again, if not, would you mind if I picked it up?
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on a diff in pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by GitBox <gi...@apache.org>.
capistrant commented on code in PR #12599:
URL: https://github.com/apache/druid/pull/12599#discussion_r914844915
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -464,6 +501,52 @@ tableName, getSerialType(), getPayloadType()
);
}
+ /**
+ * Adds the used_flag_last_updated column to the Druid segment table.
+ *
+ * This is public due to allow the UpdateTables cli tool to use for upgrade prep.
+ */
+ @Override
+ public void alterSegmentTableAddUsedFlagLastUpdated()
+ {
+ String tableName = tablesConfigSupplier.get().getSegmentsTable();
+ if (!tableHasColumn(tableName, "used_flag_last_updated")) {
Review Comment:
thinking out loud: should this column check be put into alterTable instead of this location?
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty (druid)
Posted by "maytasm (via GitHub)" <gi...@apache.org>.
maytasm commented on PR #12599:
URL: https://github.com/apache/druid/pull/12599#issuecomment-1681714225
@jon-wei @capistrant Is this PR good to merge?
@jon-wei , I tried clicking on https://github.com/apache/druid/pull/12599/files#r940734962 in your previous comment but i'm not seeing any comment when I open the link
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by GitBox <gi...@apache.org>.
capistrant commented on PR #12599:
URL: https://github.com/apache/druid/pull/12599#issuecomment-1156653270
> #12404 (currently-open PR) also contemplates doing schema changes in the metadata store. @capistrant I see you've commented in there too. Will the feature in this PR be able to use that other PR's mechanism? (Once it's merged?)
My plan is to use as much of this PRs process as is feasible. Our approaches are nearly identical for the actual alteration of the table on startup of the controlling service (overlord for that one, coordinator for mine). The difference really emerges in the migration process. The linked PR is more complicated than mine because it is replacing existing always-on functionality. That requires them to be deliberate about how they transition from querying sql the old way to the new way. They background a thread on startup that incrementally brings the table up to spec by populating rows. I could certainly implement a similar approach instead of allowing the operator to optionally make their existing unused segments compatible with the buffer period by using a druid cli tool or an adhoc sql statement.
Long story short, I don't think there is anything 12404 would/should take from my implementation. However, there is certainly shared code that I can take from them once they are merged. I will contemplate the idea of a background thread that brings the table up to spec, it would really be pretty straight forward. If there are no unused rows with null `last_used` exit immediately. Otherwise use 12404 incremental batching process to update with current date until there are no null values in the column. That would eliminate overhead/confusion for our operators
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on a diff in pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by GitBox <gi...@apache.org>.
gianm commented on code in PR #12599:
URL: https://github.com/apache/druid/pull/12599#discussion_r940711670
##########
core/src/main/java/org/apache/druid/metadata/MetadataStorageConnector.java:
##########
@@ -88,4 +88,22 @@ default void exportTable(
void createSupervisorsTable();
void deleteAllRecords(String tableName);
+
+ /**
+ * Upgrade Compatability Method.
+ *
+ * A new column, used_flag_last_updated, is added to druid_segmens table. This method alters the table to add the column to make
Review Comment:
`druid_segments` (spelling)
##########
core/src/main/java/org/apache/druid/metadata/MetadataStorageConnector.java:
##########
@@ -88,4 +88,22 @@ default void exportTable(
void createSupervisorsTable();
void deleteAllRecords(String tableName);
+
+ /**
+ * Upgrade Compatability Method.
Review Comment:
compatibility (spelling)
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataSegmentPublisher.java:
##########
@@ -73,7 +73,8 @@ public void publishSegment(final DataSegment segment) throws IOException
(segment.getShardSpec() instanceof NoneShardSpec) ? false : true,
segment.getVersion(),
true,
- jsonMapper.writeValueAsBytes(segment)
+ jsonMapper.writeValueAsBytes(segment),
+ DateTimes.nowUtc().toString()
Review Comment:
Would be tidier to call `DateTimes.nowUtc()` just once.
##########
docs/configuration/index.md:
##########
@@ -816,6 +816,7 @@ These Coordinator static configurations can be defined in the `coordinator/runti
|`druid.coordinator.kill.period`|How often to send kill tasks to the indexing service. Value must be greater than `druid.coordinator.period.indexingPeriod`. Only applies if kill is turned on.|P1D (1 Day)|
|`druid.coordinator.kill.durationToRetain`|Only applies if you set `druid.coordinator.kill.on` to `true`. This value is ignored if `druid.coordinator.kill.ignoreDurationToRetain` is `true`. Valid configurations must be a ISO8601 period. Druid will not kill unused segments whose interval end date is beyond `now - durationToRetain`. `durationToRetain` can be a negative ISO8601 period, which would result in `now - durationToRetain` to be in the future.<br /><br />Note that the `durationToRetain` parameter applies to the segment interval, not the time that the segment was last marked unused. For example, if `durationToRetain` is set to `P90D`, then a segment for a time chunk 90 days in the past is eligible for permanent deletion immediately after being marked unused.|`P90D`|
|`druid.coordinator.kill.ignoreDurationToRetain`|A way to override `druid.coordinator.kill.durationToRetain` and tell the coordinator that you do not care about the end date of unused segment intervals when it comes to killing them. If true, the coordinator considers all unused segments as eligible to be killed.|false|
+|`druid.coordinator.kill.bufferPeriod`|The amount of time that a segment must be unused before it is able to be permanently removed from metadata and deep storage. This can serve as a buffer period to prevent data loss if data ends up being needed after being marked unused.|`PT24H`|
Review Comment:
24h seems aggressive as a default. Personally I'd be more comfortable with 30 days, if it's a cluster I'm operating.
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -464,6 +501,52 @@ tableName, getSerialType(), getPayloadType()
);
}
+ /**
+ * Adds the used_flag_last_updated column to the Druid segment table.
+ *
+ * This is public due to allow the UpdateTables cli tool to use for upgrade prep.
+ */
+ @Override
+ public void alterSegmentTableAddUsedFlagLastUpdated()
+ {
+ String tableName = tablesConfigSupplier.get().getSegmentsTable();
+ if (!tableHasColumn(tableName, "used_flag_last_updated")) {
Review Comment:
So far, the only ALTERs we have are to add columns. Definitely sounds reasonable to specialize the general method in that direction. It'd also help with the log messages, since the method would become aware of what column wants to be added.
##########
services/src/main/java/org/apache/druid/cli/UpdateTables.java:
##########
@@ -0,0 +1,137 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.cli;
+
+import com.github.rvesse.airline.annotations.Command;
+import com.github.rvesse.airline.annotations.Option;
+import com.github.rvesse.airline.annotations.restrictions.Required;
+import com.google.common.collect.ImmutableList;
+import com.google.inject.Injector;
+import com.google.inject.Key;
+import com.google.inject.Module;
+import org.apache.druid.guice.DruidProcessingModule;
+import org.apache.druid.guice.JsonConfigProvider;
+import org.apache.druid.guice.QueryRunnerFactoryModule;
+import org.apache.druid.guice.QueryableModule;
+import org.apache.druid.guice.annotations.Self;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.metadata.MetadataStorageConnector;
+import org.apache.druid.metadata.MetadataStorageConnectorConfig;
+import org.apache.druid.metadata.MetadataStorageTablesConfig;
+import org.apache.druid.server.DruidNode;
+
+import java.util.List;
+
+@Command(
+ name = "metadata-update",
+ description = "Controlled update of metadta storage"
Review Comment:
metadata (spelling)
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -212,6 +213,41 @@ public Void withHandle(Handle handle)
}
}
+ /**
+ * Execute the desired ALTER statement on the desired table
+ *
+ * @param tableName The name of the table being altered
+ * @param sql ALTER statment to be executed
+ */
+ private void alterTable(final String tableName, final Iterable<String> sql)
Review Comment:
Refactor `alterEntryTable` to use this?
I like that `alterEntryTable` has nice log messages that mention the specific update that is being made, in natural language, like "Adding column: type to table[tasks]". Would be good to combine the nice log messages of that method with the genericness of this method.
##########
server/src/main/java/org/apache/druid/metadata/SqlSegmentsMetadataManager.java:
##########
@@ -993,7 +1000,7 @@ public List<Interval> inTransaction(Handle handle, TransactionStatus status)
.createQuery(
StringUtils.format(
"SELECT start, %2$send%2$s FROM %1$s WHERE dataSource = :dataSource AND "
- + "%2$send%2$s <= :end AND used = false ORDER BY start, %2$send%2$s",
+ + "%2$send%2$s <= :end AND used = false AND (used_flag_last_updated IS NULL or used_flag_last_updated <= :used_flag_last_updated) ORDER BY start, %2$send%2$s",
Review Comment:
I think we can get somewhat better behavior in the NULL case by using the created_date:
```
used = false AND ((used_flag_last_updated IS NULL AND created_date <= :used_flag_last_updated) OR used_flag_last_updated <= :used_flag_last_updated)
```
A little closer to the behavior we'd get if the `used_flag_last_updated` was actually populated.
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -824,4 +911,59 @@ public Void withHandle(Handle handle)
log.warn(e, "Exception while deleting records from table");
}
}
+
+ /**
+ * Interrogate table metadata and return true or false depending on the existance of the indicated column
+ *
+ * public visibility because DerbyConnector needs to override thanks to uppercase table and column names invalidating
+ * this implementation.
+ *
+ * @param tableName The table being interrogated
+ * @param columnName The column being looked for
+ * @return boolean indicating the existence of the column in question
+ */
+ public boolean tableHasColumn(String tableName, String columnName)
+ {
+ return getDBI().withHandle(
+ new HandleCallback<Boolean>()
+ {
+ @Override
+ public Boolean withHandle(Handle handle)
+ {
+ try {
+ if (tableExists(handle, tableName)) {
+ DatabaseMetaData dbMetaData = handle.getConnection().getMetaData();
+ ResultSet columns = dbMetaData.getColumns(
+ null,
+ null,
+ tableName,
+ columnName
+ );
+ return columns.next();
+ } else {
+ return false;
+ }
+ }
+ catch (SQLException e) {
+ return false;
+ }
+ }
+ }
+ );
+ }
+
+ /**
+ * Ensure that the segment table has the proper schema required to run Druid properly.
+ *
+ * Throws RuntimeException if the column does not exist. There is no recovering from an invalid schema,
+ * the program should crash.
+ */
+ private void validateSegmentTable()
+ {
+ if (tableHasColumn(tablesConfigSupplier.get().getSegmentsTable(), "used_flag_last_updated")) {
+ return;
+ } else {
+ throw new RuntimeException("Invalid Segment Table Schema! No used_flag_last_updated column!");
Review Comment:
Would be good to link to the docs here about how to do the migration manually. We can write the link out even though it won't be valid until after the release is made: https://druid.apache.org/docs/latest/operations/upgrade-prep.html
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1024,7 +1024,8 @@ private Set<DataSegment> announceHistoricalSegmentBatch(
.bind("partitioned", (segment.getShardSpec() instanceof NoneShardSpec) ? false : true)
.bind("version", segment.getVersion())
.bind("used", usedSegments.contains(segment))
- .bind("payload", jsonMapper.writeValueAsBytes(segment));
+ .bind("payload", jsonMapper.writeValueAsBytes(segment))
+ .bind("used_flag_last_updated", DateTimes.nowUtc().toString());
Review Comment:
For niceness, it'd be good to have this `DateTimes.nowUtc()` grabbed once so it's always going to be the same for `created_date` and `used_flag_last_updated`. Doesn't really matter, but it's tidier.
##########
core/src/main/java/org/apache/druid/metadata/MetadataStorageConnector.java:
##########
@@ -88,4 +88,22 @@ default void exportTable(
void createSupervisorsTable();
void deleteAllRecords(String tableName);
+
+ /**
+ * Upgrade Compatability Method.
+ *
+ * A new column, used_flag_last_updated, is added to druid_segmens table. This method alters the table to add the column to make
+ * a cluster's metastore tables compatible with the updated Druid codebase in 0.24.x+
+ */
+ void alterSegmentTableAddUsedFlagLastUpdated();
+
+ /**
+ * Upgrade Compatability Method.
Review Comment:
compatibility (spelling)
##########
docs/operations/upgrade-prep.md:
##########
@@ -0,0 +1,114 @@
+---
+id: upgrade-prep
+title: "Upgrade Prep"
+---
+
+<!--
+ ~ Licensed to the Apache Software Foundation (ASF) under one
+ ~ or more contributor license agreements. See the NOTICE file
+ ~ distributed with this work for additional information
+ ~ regarding copyright ownership. The ASF licenses this file
+ ~ to you under the Apache License, Version 2.0 (the
+ ~ "License"); you may not use this file except in compliance
+ ~ with the License. You may obtain a copy of the License at
+ ~
+ ~ http://www.apache.org/licenses/LICENSE-2.0
+ ~
+ ~ Unless required by applicable law or agreed to in writing,
+ ~ software distributed under the License is distributed on an
+ ~ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ ~ KIND, either express or implied. See the License for the
+ ~ specific language governing permissions and limitations
+ ~ under the License.
+ -->
+
+## Upgrade to `0.24+` from `0.23` and earlier
+
+### Altering segments table
+
+**If you have set `druid.metadata.storage.connector.createTables` to `true` (which is the default), and your metadata connect user has DDL privileges, you can disregard this section. You are urged to still evaluate the optional section below**
+
+**The coordinator and overlord services will fail if you do not execute this change prior to the upgrade**
+
+A new column, `used_flag_last_updated`, is needed in the segments table to support new
+segment killing functionality. You can manually alter the table, or you can use
+a CLI tool to perform the update.
+
+#### CLI tool
+
+Druid provides a `metadata-update` tool for updating Druid's metadata tables.
+
+In the example commands below:
+
+- `lib` is the Druid lib directory
+- `extensions` is the Druid extensions directory
+- `base` corresponds to the value of `druid.metadata.storage.tables.base` in the configuration, `druid` by default.
+- The `--connectURI` parameter corresponds to the value of `druid.metadata.storage.connector.connectURI`.
+- The `--user` parameter corresponds to the value of `druid.metadata.storage.connector.user`.
+- The `--password` parameter corresponds to the value of `druid.metadata.storage.connector.password`.
+- The `--action` parameter corresponds to the update action you are executing. In this case it is: `add-last-used-to-segments`
+
+##### MySQL
+
+```bash
+cd ${DRUID_ROOT}
+java -classpath "lib/*" -Dlog4j.configurationFile=conf/druid/cluster/_common/log4j2.xml -Ddruid.extensions.directory="extensions" -Ddruid.extensions.loadList=[\"mysql-metadata-storage\"] -Ddruid.metadata.storage.type=mysql org.apache.druid.cli.Main tools metadata-update --connectURI="<mysql-uri>" --user <user> --password <pass> --base druid --action add-used-flag-last-updated-to-segments
+```
+
+##### PostgreSQL
+
+```bash
+cd ${DRUID_ROOT}
+java -classpath "lib/*" -Dlog4j.configurationFile=conf/druid/cluster/_common/log4j2.xml -Ddruid.extensions.directory="extensions" -Ddruid.extensions.loadList=[\"postgresql-metadata-storage\"] -Ddruid.metadata.storage.type=postgresql org.apache.druid.cli.Main tools metadata-update --connectURI="<postgresql-uri>" --user <user> --password <pass> --base druid --action add-used-flag-last-updated-to-segments
+```
+
+
+#### Manual ALTER TABLE
+
+```SQL
+ALTER TABLE druid_segments
+ADD used_flag_last_updated varchar(255);
+```
+
+### Populating `used_flag_last_updated` column of the segments table after upgrade (Optional)
Review Comment:
I suggest adding a name here so we can link to this specific section more easily. Like:
```
<a name="populate-used-flag-last-updated"></a>
```
It goes before the `### Populating...` line.
First place I'd like to it is the text "optional section below" above. That can be done like:
```
[optional section below](#populate-used-flag-last-updated)
```
##########
docs/configuration/index.md:
##########
@@ -816,6 +816,7 @@ These Coordinator static configurations can be defined in the `coordinator/runti
|`druid.coordinator.kill.period`|How often to send kill tasks to the indexing service. Value must be greater than `druid.coordinator.period.indexingPeriod`. Only applies if kill is turned on.|P1D (1 Day)|
|`druid.coordinator.kill.durationToRetain`|Only applies if you set `druid.coordinator.kill.on` to `true`. This value is ignored if `druid.coordinator.kill.ignoreDurationToRetain` is `true`. Valid configurations must be a ISO8601 period. Druid will not kill unused segments whose interval end date is beyond `now - durationToRetain`. `durationToRetain` can be a negative ISO8601 period, which would result in `now - durationToRetain` to be in the future.<br /><br />Note that the `durationToRetain` parameter applies to the segment interval, not the time that the segment was last marked unused. For example, if `durationToRetain` is set to `P90D`, then a segment for a time chunk 90 days in the past is eligible for permanent deletion immediately after being marked unused.|`P90D`|
|`druid.coordinator.kill.ignoreDurationToRetain`|A way to override `druid.coordinator.kill.durationToRetain` and tell the coordinator that you do not care about the end date of unused segment intervals when it comes to killing them. If true, the coordinator considers all unused segments as eligible to be killed.|false|
+|`druid.coordinator.kill.bufferPeriod`|The amount of time that a segment must be unused before it is able to be permanently removed from metadata and deep storage. This can serve as a buffer period to prevent data loss if data ends up being needed after being marked unused.|`PT24H`|
Review Comment:
We should describe that if you updated from an earlier version, this won't apply to all segments. We can also link from this to the doc about how to do the manual update. Maybe like:
> The amount of time that a segment must be unused before it is able to be permanently removed from metadata and deep storage. This can serve as a buffer period to prevent data loss if data ends up being needed after being marked unused.`<br /><br />`Segments created with Druid 0.23.x and earlier do not have the necessary metadata required to respect this configuration, and may be permanently removed even if marked used or unused within the buffer period. To address this, you can `[populate this field in existing segment records](../operations/upgrade-prep.md#populate-used-flag-last-updated)` using a command-line tool or SQL command.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on PR #12599:
URL: https://github.com/apache/druid/pull/12599#issuecomment-1331809192
@capistrant - are you still working on this PR? Asking since this is a really useful feature. And after this, we can enable the auto-killing of unused segments.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty (druid)
Posted by "maytasm (via GitHub)" <gi...@apache.org>.
maytasm commented on PR #12599:
URL: https://github.com/apache/druid/pull/12599#issuecomment-1907100178
Should used_flag_last_updated column be part of the index in the segment table?
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by GitBox <gi...@apache.org>.
gianm commented on PR #12599:
URL: https://github.com/apache/druid/pull/12599#issuecomment-1154576625
#12404 (currently-open PR) also contemplates doing schema changes in the metadata store. @capistrant I see you've commented in there too. Will the feature in this PR be able to use that other PR's mechanism? (Once it's merged?)
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by "abhishekagarwal87 (via GitHub)" <gi...@apache.org>.
abhishekagarwal87 commented on PR #12599:
URL: https://github.com/apache/druid/pull/12599#issuecomment-1429137147
@capistrant - Just checking to see if you are still planning to take this through. If not, are you open to someone else picking it up?
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on a diff in pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by GitBox <gi...@apache.org>.
capistrant commented on code in PR #12599:
URL: https://github.com/apache/druid/pull/12599#discussion_r891702066
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -765,4 +852,52 @@ public Void withHandle(Handle handle)
log.warn(e, "Exception while deleting records from table");
}
}
+
+ /**
+ * Interrogate table metadata and return true or false depending on the existance of the indicated column
+ *
+ * @param tableName The table being interrogated
+ * @param columnName The column being looked for
+ * @return boolean indicating the existence of the column in question
+ */
+ private boolean tableHasColumn(String tableName, String columnName)
Review Comment:
what if table doesn't exist?
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by GitBox <gi...@apache.org>.
capistrant commented on PR #12599:
URL: https://github.com/apache/druid/pull/12599#issuecomment-1164495871
Todo reminder for myself: evaluate feasibility of integration test of UpdateTable.java cli tool
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on a diff in pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by GitBox <gi...@apache.org>.
capistrant commented on code in PR #12599:
URL: https://github.com/apache/druid/pull/12599#discussion_r891706572
##########
server/src/test/java/org/apache/druid/metadata/SqlSegmentsMetadataManagerTest.java:
##########
@@ -363,29 +372,60 @@ public void testPollWithCorruptedSegment()
);
}
+ // TODO
Review Comment:
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on a diff in pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by GitBox <gi...@apache.org>.
capistrant commented on code in PR #12599:
URL: https://github.com/apache/druid/pull/12599#discussion_r891701422
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -408,6 +447,50 @@ tableName, getSerialType(), getPayloadType()
);
}
+ /**
+ * Adds the last_used column to the Druid segment table.
+ *
+ * This is public due to allow the UpdateTables cli tool to use for upgrade prep.
+ */
+ @Override
+ public void alterSegmentTableAddLastUsed()
+ {
+ String tableName = tablesConfigSupplier.get().getSegmentsTable();
+ if (!tableHasColumn(tableName, "last_used")) {
+ log.info("Adding last_used column to %s", tableName);
+ alterTable(
+ tableName,
+ ImmutableList.of(
+ StringUtils.format(
+ "ALTER TABLE %1$s \n"
+ + "ADD last_used varchar(255)",
+ tableName
+ )
+ )
+ );
+ } else {
+ log.info("%s already has last_used column", tableName);
+ }
+ }
+
+ /**
+ * Populates the last_used column for all unused segments in the Druid segment table.
+ *
+ * The current UTC timestamp string is used for the content of each column.
+ * This is public due to allow the UpdateTables cli tool to use in a optional post-upgrade action.
+ */
+ @Override
+ public void updateSegmentTablePopulateLastUsed()
Review Comment:
I suspect we want an existence check on table
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by GitBox <gi...@apache.org>.
capistrant commented on PR #12599:
URL: https://github.com/apache/druid/pull/12599#issuecomment-1156418937
> As to the main change: what do you think of calling the new column `last_updated`? IMO, that's clearer, since `last_used` sounds like it might be the last time the segment was loaded or queried or something like that.
>
> Generally, this change would be very welcome. Measuring time-to-kill from the last update time makes more sense than measuring it from the data interval.
I'm certainly open to different naming. `last_changed` spooks me because there could be a future where we change a row for some other reason than flipping the `used` flag. Perhaps `used_last_updated`, `[marked-]unused_since` (although this one might require us to re-think what the timestamp is when `used==true`.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty (druid)
Posted by "jon-wei (via GitHub)" <gi...@apache.org>.
jon-wei merged PR #12599:
URL: https://github.com/apache/druid/pull/12599
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on a diff in pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by GitBox <gi...@apache.org>.
capistrant commented on code in PR #12599:
URL: https://github.com/apache/druid/pull/12599#discussion_r914850385
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -464,6 +501,52 @@ tableName, getSerialType(), getPayloadType()
);
}
+ /**
+ * Adds the used_flag_last_updated column to the Druid segment table.
+ *
+ * This is public due to allow the UpdateTables cli tool to use for upgrade prep.
+ */
+ @Override
+ public void alterSegmentTableAddUsedFlagLastUpdated()
+ {
+ String tableName = tablesConfigSupplier.get().getSegmentsTable();
+ if (!tableHasColumn(tableName, "used_flag_last_updated")) {
+ log.info("Adding used_flag_last_updated column to %s", tableName);
+ alterTable(
+ tableName,
+ ImmutableList.of(
+ StringUtils.format(
+ "ALTER TABLE %1$s \n"
+ + "ADD used_flag_last_updated varchar(255)",
+ tableName
+ )
+ )
+ );
+ } else {
+ log.info("%s already has used_flag_last_updated column", tableName);
+ }
+ }
+
+ /**
+ * Populates the used_flag_last_updated column for all unused segments in the Druid segment table.
+ *
+ * The current UTC timestamp string is used for the content of each column.
+ * This is public due to allow the UpdateTables cli tool to use in a optional post-upgrade action.
+ */
+ @Override
+ public void updateSegmentTablePopulateUsedFlagLastUpdated()
Review Comment:
thinking out loud: Might make sense to add batching capability here. an int batching size that could break up the UPDATE command to avoid updating many rows in one action. For instance, an operator with 6 million unused segments may not want to do that kind of db update all at once. something like a batch of 10k might be more palatable to them. I do warn the operator about the single transaction in the docs regarding the cli tool update that calls this method so people *should* not be caught off guard by this if they read the doc before using the cli tool (which is the only thing that calls this method as of now)
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] gianm commented on pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by GitBox <gi...@apache.org>.
gianm commented on PR #12599:
URL: https://github.com/apache/druid/pull/12599#issuecomment-1154579647
Alternatively, should #12404 use the schema migration mechanism from this patch? I haven't reviewed both closely. I'm not sure which mechanism is preferable. Curious what @AmatyaAvadhanula and @imply-cheddar and @kfaraz think too.
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on a diff in pull request #12599: Add a configurable bufferPeriod between when a segment is marked unused and deleted by KillUnusedSegments duty
Posted by GitBox <gi...@apache.org>.
capistrant commented on code in PR #12599:
URL: https://github.com/apache/druid/pull/12599#discussion_r891701895
##########
server/src/main/java/org/apache/druid/metadata/SQLMetadataConnector.java:
##########
@@ -408,6 +447,50 @@ tableName, getSerialType(), getPayloadType()
);
}
+ /**
+ * Adds the last_used column to the Druid segment table.
+ *
+ * This is public due to allow the UpdateTables cli tool to use for upgrade prep.
+ */
+ @Override
+ public void alterSegmentTableAddLastUsed()
+ {
+ String tableName = tablesConfigSupplier.get().getSegmentsTable();
+ if (!tableHasColumn(tableName, "last_used")) {
Review Comment:
what if table doesn't exist?
--
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@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org