kfaraz commented on code in PR #14639:
URL: https://github.com/apache/druid/pull/14639#discussion_r1271376502
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1773,32 +1773,32 @@ public Void inTransaction(Handle handle,
TransactionStatus transactionStatus) th
@Override
public void deleteSegments(final Set<DataSegment> segments)
{
- connector.getDBI().inTransaction(
- new TransactionCallback<Void>()
- {
- @Override
- public Void inTransaction(Handle handle, TransactionStatus
transactionStatus)
- {
- int segmentSize = segments.size();
- String dataSource = "";
- for (final DataSegment segment : segments) {
- dataSource = segment.getDataSource();
- deleteSegment(handle, segment);
- }
- log.debugSegments(segments, "Delete the metadata of segments");
- log.info("Removed [%d] segments from metadata storage for
dataSource [%s]!", segmentSize, dataSource);
+ if (segments.isEmpty()) {
+ log.info("Removed [0] segments from metadata storage for dataSource
[\"\"]!");
Review Comment:
We need not adhere to the older log message in this case.
```suggestion
log.info("No segments to delete.");
```
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1817,7 +1817,6 @@ private void updatePayload(final Handle handle, final
DataSegment segment) throw
throw e;
}
}
-
Review Comment:
Nit: missing newline
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1773,32 +1773,32 @@ public Void inTransaction(Handle handle,
TransactionStatus transactionStatus) th
@Override
public void deleteSegments(final Set<DataSegment> segments)
{
- connector.getDBI().inTransaction(
- new TransactionCallback<Void>()
- {
- @Override
- public Void inTransaction(Handle handle, TransactionStatus
transactionStatus)
- {
- int segmentSize = segments.size();
- String dataSource = "";
- for (final DataSegment segment : segments) {
- dataSource = segment.getDataSource();
- deleteSegment(handle, segment);
- }
- log.debugSegments(segments, "Delete the metadata of segments");
- log.info("Removed [%d] segments from metadata storage for
dataSource [%s]!", segmentSize, dataSource);
+ if (segments.isEmpty()) {
+ log.info("Removed [0] segments from metadata storage for dataSource
[\"\"]!");
+ return;
+ }
- return null;
+ final int segmentSize = segments.size();
+ final String deleteSql = StringUtils.format("DELETE from %s WHERE id =
:id", dbTables.getSegmentsTable());
+ final String dataSource =
segments.stream().findFirst().map(DataSegment::getDataSource).get();
+
+ // generate the IDs outside the transaction block
+ final List<String> ids = segments.stream().map(s ->
s.getId().toString()).collect(Collectors.toList());
+
+ connector.getDBI().inTransaction((TransactionCallback<Void>) (handle,
transactionStatus) -> {
Review Comment:
```suggestion
int numDeletedSegments = connector.getDBI().inTransaction((handle,
transactionStatus) -> {
```
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1773,32 +1773,32 @@ public Void inTransaction(Handle handle,
TransactionStatus transactionStatus) th
@Override
public void deleteSegments(final Set<DataSegment> segments)
{
- connector.getDBI().inTransaction(
- new TransactionCallback<Void>()
- {
- @Override
- public Void inTransaction(Handle handle, TransactionStatus
transactionStatus)
- {
- int segmentSize = segments.size();
- String dataSource = "";
- for (final DataSegment segment : segments) {
- dataSource = segment.getDataSource();
- deleteSegment(handle, segment);
- }
- log.debugSegments(segments, "Delete the metadata of segments");
- log.info("Removed [%d] segments from metadata storage for
dataSource [%s]!", segmentSize, dataSource);
+ if (segments.isEmpty()) {
+ log.info("Removed [0] segments from metadata storage for dataSource
[\"\"]!");
+ return;
+ }
- return null;
+ final int segmentSize = segments.size();
+ final String deleteSql = StringUtils.format("DELETE from %s WHERE id =
:id", dbTables.getSegmentsTable());
+ final String dataSource =
segments.stream().findFirst().map(DataSegment::getDataSource).get();
+
+ // generate the IDs outside the transaction block
+ final List<String> ids = segments.stream().map(s ->
s.getId().toString()).collect(Collectors.toList());
+
+ connector.getDBI().inTransaction((TransactionCallback<Void>) (handle,
transactionStatus) -> {
+ final PreparedBatch batch = handle.prepareBatch(deleteSql);
+
+ for (final String id : ids) {
+ batch.bind("id", id).add();
}
+
+ batch.execute();
+ return null;
Review Comment:
```suggestion
int[] deletedRows = batch.execute();
return Arrays.stream(deletedRows).sum()
```
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1773,32 +1773,32 @@ public Void inTransaction(Handle handle,
TransactionStatus transactionStatus) th
@Override
public void deleteSegments(final Set<DataSegment> segments)
{
- connector.getDBI().inTransaction(
- new TransactionCallback<Void>()
- {
- @Override
- public Void inTransaction(Handle handle, TransactionStatus
transactionStatus)
- {
- int segmentSize = segments.size();
- String dataSource = "";
- for (final DataSegment segment : segments) {
- dataSource = segment.getDataSource();
- deleteSegment(handle, segment);
- }
- log.debugSegments(segments, "Delete the metadata of segments");
- log.info("Removed [%d] segments from metadata storage for
dataSource [%s]!", segmentSize, dataSource);
+ if (segments.isEmpty()) {
+ log.info("Removed [0] segments from metadata storage for dataSource
[\"\"]!");
+ return;
+ }
- return null;
+ final int segmentSize = segments.size();
+ final String deleteSql = StringUtils.format("DELETE from %s WHERE id =
:id", dbTables.getSegmentsTable());
+ final String dataSource =
segments.stream().findFirst().map(DataSegment::getDataSource).get();
+
+ // generate the IDs outside the transaction block
+ final List<String> ids = segments.stream().map(s ->
s.getId().toString()).collect(Collectors.toList());
+
+ connector.getDBI().inTransaction((TransactionCallback<Void>) (handle,
transactionStatus) -> {
+ final PreparedBatch batch = handle.prepareBatch(deleteSql);
+
+ for (final String id : ids) {
+ batch.bind("id", id).add();
}
+
+ batch.execute();
+ return null;
}
);
- }
- private void deleteSegment(final Handle handle, final DataSegment segment)
- {
- handle.createStatement(StringUtils.format("DELETE from %s WHERE id = :id",
dbTables.getSegmentsTable()))
- .bind("id", segment.getId().toString())
- .execute();
+ log.debugSegments(segments, "Delete the metadata of segments");
+ log.info("Removed [%d] segments from metadata storage for dataSource
[%s]!", segmentSize, dataSource);
Review Comment:
```suggestion
log.info("Deleted [%d] segments from metadata storage for dataSource
[%s].", numDeletedSegments, dataSource);
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
