-
Notifications
You must be signed in to change notification settings - Fork 414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OAK-8413 Use the new Azure SDK in the Azure Segment Store #1748
base: trunk
Are you sure you want to change the base?
Conversation
…to issue/OAK-8413 # Conflicts: # oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/tool/SegmentCopy.java # oak-segment-azure/src/test/java/org/apache/jackrabbit/oak/segment/azure/AzureSegmentStoreServiceTest.java
@ierandra, the build fails with the following error in Oak Segment Azure: |
@dulceanu I was using |
@ierandra, yes, the versions need to be updated so that the build passes without skipping the baseline check. |
… package versions
Now |
@dulceanu I fixed all the tests. build should be ok now. |
This doesn't seem to compile (after applying the diff to oak trunk):
...etc... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #1748 (comment)
…to issue/OAK-8413 # Conflicts: # oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzureArchiveManager.java # oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzureJournalFile.java # oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/tool/ToolUtils.java
@reschke I updated the PR with changes from apache/jackrabbit-oak trunk and should be fine now |
It does indeed; thanks. |
This looks a bit weird:
are we exporting these packages? This looks wrong...:
|
</Export-Package> | ||
<Embed-Dependency> | ||
azure-storage, | ||
azure-keyvault-core, | ||
azure-core, | ||
azure-identity, | ||
azure-json, | ||
azure-xml, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arer we embedding the new SDK? Why?
(We embedded the old one due to the Guava dependency issue, but this should not be needed here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried without and I got the following error at build:
[ERROR] Bundle oak-segment-azure:1.71-ierandra-T20241104112150-3ccdb109e0 is importing package(s) com.azure.xml in start level 15 but no bundle is exporting these for that start level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the oak-it-osgi tests? In which case we may have to add the dependency there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened https://issues.apache.org/jira/browse/OAK-11236 - the embedding/exporting issues started earlier. |
…refix and missing metadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this adds many new files, please make sure that all of them just use LF as line ends (otherwise fixing this will cause ugly diffs), and check with "-Prat" that all licenses are ok.
oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/util/package-info.java
Show resolved
Hide resolved
The build on my machine fails because of baseline plugin
|
@smiroslav I configure it back to 2.0.0. should work now |
...ure/src/main/java/org/apache/jackrabbit/oak/segment/azure/v8/AzureSegmentStoreServiceV8.java
Outdated
Show resolved
Hide resolved
...ure/src/main/java/org/apache/jackrabbit/oak/segment/azure/v8/AzureSegmentStoreServiceV8.java
Outdated
Show resolved
Hide resolved
...ure/src/main/java/org/apache/jackrabbit/oak/segment/azure/v8/AzureSegmentStoreServiceV8.java
Outdated
Show resolved
Hide resolved
...ure/src/main/java/org/apache/jackrabbit/oak/segment/azure/v8/AzureSegmentStoreServiceV8.java
Outdated
Show resolved
Hide resolved
...ure/src/main/java/org/apache/jackrabbit/oak/segment/azure/v8/AzureSegmentStoreServiceV8.java
Outdated
Show resolved
Hide resolved
...ent-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzurePersistenceManager.java
Show resolved
Hide resolved
...ure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzureHttpRequestLoggingPolicy.java
Show resolved
Hide resolved
requestOptions.setMaximumExecutionTimeInMs(LEASE_RENEWAL_TIMEOUT_MS); | ||
requestOptions.setRetryPolicyFactory(new RetryNoRetry()); | ||
blob.renewLease(AccessCondition.generateLeaseCondition(leaseId), requestOptions, null); | ||
leaseId = leaseClient.renewLeaseWithResponse((RequestConditions) null, Duration.ofMillis(LEASE_RENEWAL_TIMEOUT_MS), Context.NONE).getValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous implementation configured request options with RetryNoRetry
.
How is it achieved here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on what I looked the solution is to add a 3rd azure connection only for the lease.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzureRepositoryLock.java
Outdated
Show resolved
Hide resolved
oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/tool/AzureCheck.java
Show resolved
Hide resolved
...nt-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/util/AzureRequestOptions.java
Outdated
Show resolved
Hide resolved
...nt-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/util/AzureRequestOptions.java
Outdated
Show resolved
Hide resolved
...nt-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/util/AzureRequestOptions.java
Outdated
Show resolved
Hide resolved
return new RequestRetryOptions(RetryPolicyType.EXPONENTIAL, | ||
retryAttempts, | ||
timeoutExecution, | ||
timeoutIntervalToMs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the intent of TIMEOUT_INTERVAL_PROP
was to specify max execution time for one retry
From the Java documentation
Sets the timeout to use when making this request.
The server timeout interval begins at the time that the complete request has been received by the service, and the server begins processing the response. If the timeout interval elapses before the response is returned to the client, the operation times out. The timeout interval resets with each retry, if the request is retried.
You can check the intent of the properties in the linked below nad map it to the corresponding ones in v12
timeoutIntervalToMs
should be the third argument.
https://learn.microsoft.com/en-us/java/api/com.azure.storage.common.policy.requestretryoptions?view=azure-java-stable#com-azure-storage-common-policy-requestretryoptions-requestretryoptions(com-azure-storage-common-policy-retrypolicytype-java-lang-integer-java-lang-integer-java-lang-long-java-lang-long-java-lang-string)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed them. Should be fine now.
...ure/src/test/java/org/apache/jackrabbit/oak/segment/azure/AzureSegmentArchiveWriterTest.java
Outdated
Show resolved
Hide resolved
...ure/src/test/java/org/apache/jackrabbit/oak/segment/azure/AzureSegmentArchiveWriterTest.java
Outdated
Show resolved
Hide resolved
...ure/src/test/java/org/apache/jackrabbit/oak/segment/azure/AzureSegmentArchiveWriterTest.java
Show resolved
Hide resolved
...gment-azure/src/test/java/org/apache/jackrabbit/oak/segment/azure/MockAzureHttpResponse.java
Show resolved
Hide resolved
@ierandra, I would prefer if you let me resolve the PR comments that I have started. |
return getRetryOptionsDefault(null); | ||
} | ||
|
||
public static RequestRetryOptions getRetryOptionsDefault(String secondaryHost) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some system properties can be reused, and those which can not should be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please be more specific. now sure what to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the API RequestRetryOptions
constructor has the following arguments:
retryPolicyType
- this one should beRetryPolicyType.FIXED
✅maxTries
- this should come from RETRY_ATTEMPTS_PROP ("segment.azure.retry.attempts") ✅tryTimeoutInSeconds
- this would be max timeout for one retry, initialised with TIMEOUT_INTERVAL_PROP ("segment.timeout.interval") ❌ currently in the code it takes value from TIMEOUT_EXECUTION_PROPretryDelayInMs
- should come from RETRY_BACKOFF_PROP ("segment.azure.retry.backoff") ✅maxRetryDelayInMs
- probably the same value as forretryDelayInMs
can be used
Unused system properties should be deleted from AzureRequestOptions
This link explains retry options in detail
https://learn.microsoft.com/en-us/azure/storage/blobs/storage-retry-policy-java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
…to issue/OAK-8413 # Conflicts: # oak-segment-azure/src/main/java/org/apache/jackrabbit/oak/segment/azure/AzureRepositoryLock.java # oak-segment-azure/src/test/java/org/apache/jackrabbit/oak/segment/azure/tool/SegmentCopyAzureServicePrincipalToTarTest.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm only looking into POMs here.
It is unclear why apparently apparently all new dependencies get embedded. What's the reason for that?
As I mention in my previous comment if I don't do it if fails at build phase for CQ: |
@smiroslav I fixed all the comments. could you please take a look? |
No description provided.