nsivabalan commented on code in PR #14022:
URL: https://github.com/apache/hudi/pull/14022#discussion_r2392877566
##########
hudi-aws/src/main/java/org/apache/hudi/aws/transaction/lock/S3StorageLockClient.java:
##########
@@ -143,7 +143,7 @@ public Pair<LockUpsertResult, Option<StorageLockFile>>
tryUpsertLockFile(Storage
} catch (S3Exception e) {
result = handleUpsertS3Exception(e);
} catch (AwsServiceException | SdkClientException e) {
- logger.warn("OwnerId: {}, Unexpected SDK error while writing lock file:
{}", ownerId, lockFilePath, e);
+ logger.error("OwnerId: {}, Unexpected SDK error while writing lock file:
{}", ownerId, lockFilePath, e);
Review Comment:
hey @alexr17 : why we don't throw if not for lockRenewal here?
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/execution/CopyOnWriteInsertHandler.java:
##########
@@ -88,7 +88,7 @@ public void consume(HoodieInsertValueGenResult<HoodieRecord>
genResult) {
return;
}
} catch (IOException e) {
- LOG.warn("Writing record should be ignore " + record, e);
+ LOG.error("Writing record should be ignore {}", record, e);
Review Comment:
not related to this patch.
but not sure if we can swallow the exception here and move on.
we might need to throw an exception if `shouldIgnore` failed.
we can only ignore if `shouldIgnore` returns true.
##########
hudi-aws/src/main/java/org/apache/hudi/aws/transaction/lock/S3StorageLockClient.java:
##########
@@ -143,7 +143,7 @@ public Pair<LockUpsertResult, Option<StorageLockFile>>
tryUpsertLockFile(Storage
} catch (S3Exception e) {
result = handleUpsertS3Exception(e);
} catch (AwsServiceException | SdkClientException e) {
- logger.warn("OwnerId: {}, Unexpected SDK error while writing lock file:
{}", ownerId, lockFilePath, e);
+ logger.error("OwnerId: {}, Unexpected SDK error while writing lock file:
{}", ownerId, lockFilePath, e);
Review Comment:
btw, we are taking a look at all warn logs and trying to move them to info
or error.
Essentially, we should have very minimal warn logs, as thats the default log
level and user exp should be good.
can you take a look at changes in S3 storage based lock provider classes in
this patch and share any feedback you have.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/upgrade/SevenToEightUpgradeHandler.java:
##########
@@ -306,7 +306,7 @@ static boolean upgradeActiveTimelineInstant(HoodieInstant
instant, String origin
try {
return rewriteTimelineV1InstantFileToV2Format(instant, metaClient,
originalFileName, replacedFileName, commitMetadataSerDeV1,
commitMetadataSerDeV2, activeTimelineV2);
} catch (IOException e) {
- LOG.warn("Can not to complete the upgrade from version seven to version
eight. The reason for failure is {}", e.getMessage());
+ LOG.warn("Can not to complete the upgrade from version seven to version
eight", e);
Review Comment:
lets file a ticket and fix this. if active timeline rewrite fails, we can't
proceed w/ upgrade, but need to throw exception that upgrade failed.
##########
hudi-aws/src/main/java/org/apache/hudi/aws/transaction/lock/S3StorageLockClient.java:
##########
@@ -143,7 +143,7 @@ public Pair<LockUpsertResult, Option<StorageLockFile>>
tryUpsertLockFile(Storage
} catch (S3Exception e) {
result = handleUpsertS3Exception(e);
} catch (AwsServiceException | SdkClientException e) {
- logger.warn("OwnerId: {}, Unexpected SDK error while writing lock file:
{}", ownerId, lockFilePath, e);
+ logger.error("OwnerId: {}, Unexpected SDK error while writing lock file:
{}", ownerId, lockFilePath, e);
Review Comment:
hey @alexr17 : why we don't throw if not for lockRenewal here?
##########
hudi-aws/src/main/java/org/apache/hudi/aws/transaction/lock/S3StorageLockClient.java:
##########
@@ -143,7 +143,7 @@ public Pair<LockUpsertResult, Option<StorageLockFile>>
tryUpsertLockFile(Storage
} catch (S3Exception e) {
result = handleUpsertS3Exception(e);
} catch (AwsServiceException | SdkClientException e) {
- logger.warn("OwnerId: {}, Unexpected SDK error while writing lock file:
{}", ownerId, lockFilePath, e);
+ logger.error("OwnerId: {}, Unexpected SDK error while writing lock file:
{}", ownerId, lockFilePath, e);
Review Comment:
btw, we are taking a look at all warn logs and trying to move them to info
or error.
Essentially, we should have very minimal warn logs, as thats the default log
level and user exp should be good.
can you take a look at changes in S3 storage based lock provider classes in
this patch and share any feedback you have.
--
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]
