abhishekrb19 commented on code in PR #14639:
URL: https://github.com/apache/druid/pull/14639#discussion_r1271175394
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1760,8 +1761,22 @@ public void updateSegmentMetadata(final Set<DataSegment>
segments)
@Override
public Void inTransaction(Handle handle, TransactionStatus
transactionStatus) throws Exception
{
+ final String updatePayload = StringUtils.format("UPDATE %s SET
payload = :payload WHERE id = :id", dbTables.getSegmentsTable());
+ final PreparedBatch batch = handle.prepareBatch(updatePayload);
+
for (final DataSegment segment : segments) {
- updatePayload(handle, segment);
+ String id = segment.getId().toString();
+ byte[] payload = jsonMapper.writeValueAsBytes(segment);
Review Comment:
I think keeping the `inTransaction` code block concise and focused to only
transaction-related things will generally reduce the DB transaction duration
and make the code a bit more readable. For example, this serialization can be
moved above `connector.getDBI().inTransaction()` and collected in a list and
here we can just iterate over the list and do a simple `bind()`
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1780,11 +1795,16 @@ public void deleteSegments(final Set<DataSegment>
segments)
public Void inTransaction(Handle handle, TransactionStatus
transactionStatus)
{
int segmentSize = segments.size();
- String dataSource = "";
+ String dataSource =
segments.stream().findAny().map(DataSegment::getDataSource).orElse("?");
Review Comment:
Do we need this `.orElse("?")`? Also, I think we can move this line and the
query declaration outside the scope of the transaction.
##########
server/src/test/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinatorTest.java:
##########
@@ -1413,6 +1425,46 @@ public void testDeleteSegmentsInMetaDataStorage() throws
IOException
);
}
+ @Test
+ public void testUpdateSegmentsInMetaDataStorage() throws IOException
Review Comment:
Thanks for adding this unit test!
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1760,8 +1761,22 @@ public void updateSegmentMetadata(final Set<DataSegment>
segments)
@Override
public Void inTransaction(Handle handle, TransactionStatus
transactionStatus) throws Exception
{
+ final String updatePayload = StringUtils.format("UPDATE %s SET
payload = :payload WHERE id = :id", dbTables.getSegmentsTable());
+ final PreparedBatch batch = handle.prepareBatch(updatePayload);
+
for (final DataSegment segment : segments) {
Review Comment:
Do we know if the list of input segments to `updateSegmentMetadata` is
bounded somewhere? If the number of segments is very large or unbounded, then a
single batch transaction for the entire set can be problematic. So we would
probably have to break down into multiple batch transactions of reasonable size
each. What do you think?
##########
server/src/main/java/org/apache/druid/metadata/IndexerSQLMetadataStorageCoordinator.java:
##########
@@ -1760,8 +1761,22 @@ public void updateSegmentMetadata(final Set<DataSegment>
segments)
@Override
public Void inTransaction(Handle handle, TransactionStatus
transactionStatus) throws Exception
{
+ final String updatePayload = StringUtils.format("UPDATE %s SET
payload = :payload WHERE id = :id", dbTables.getSegmentsTable());
+ final PreparedBatch batch = handle.prepareBatch(updatePayload);
+
for (final DataSegment segment : segments) {
- updatePayload(handle, segment);
+ String id = segment.getId().toString();
+ byte[] payload = jsonMapper.writeValueAsBytes(segment);
+
+ batch.bind("id", id).bind("payload", payload).add();
+ }
+
+ try {
+ batch.execute();
+ }
+ catch (DBIException e) {
+ log.error(e, "Exception inserting into DB");
Review Comment:
```suggestion
log.error(e, "Exception updating segment metadata in DB");
```
--
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]
