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 2020/03/17 08:16:57 UTC
[GitHub] [druid] zachjsh opened a new pull request #9525: Azure deep storage
does not work with datasource name containing non-ASCII chars
zachjsh opened a new pull request #9525: Azure deep storage does not work with datasource name containing non-ASCII chars
URL: https://github.com/apache/druid/pull/9525
Fixes #9515
### Description
Fixed a bug where recording the segment file location fails when
using Azure Deep Storage, if the datasource has any special
characters. Verified fix with manual test.
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] 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/licenses.yaml)
- [ ] 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.
- [ ] 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] maytasm3 commented on issue #9525: Azure deep storage does
not work with datasource name containing non-ASCII chars
Posted by GitBox <gi...@apache.org>.
maytasm3 commented on issue #9525: Azure deep storage does not work with datasource name containing non-ASCII chars
URL: https://github.com/apache/druid/pull/9525#issuecomment-599938840
If this PR https://github.com/apache/druid/pull/9501 goes in, you can also run the integration test for Azure which will verify your fix.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9525:
Azure deep storage does not work with datasource name containing non-ASCII
chars
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9525: Azure deep storage does not work with datasource name containing non-ASCII chars
URL: https://github.com/apache/druid/pull/9525#discussion_r395270761
##########
File path: extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureDataSegmentPusher.java
##########
@@ -153,14 +153,7 @@ public DataSegment push(final File indexFilesDir, final DataSegment segment, fin
@Override
public Map<String, Object> makeLoadSpec(URI uri)
{
- return ImmutableMap.of(
- "type",
- AzureStorageDruidModule.SCHEME,
- "containerName",
- segmentConfig.getContainer(),
- "blobPath",
- uri.toString()
- );
+ return makeLoadSpec(uri.toString());
Review comment:
It looks like hadoop indexing would still call this method so it might have issues with funny characters, but I don't think it is worth refactoring to fix the issue at this point.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] zachjsh commented on a change in pull request #9525: Azure
deep storage does not work with datasource name containing non-ASCII chars
Posted by GitBox <gi...@apache.org>.
zachjsh commented on a change in pull request #9525: Azure deep storage does not work with datasource name containing non-ASCII chars
URL: https://github.com/apache/druid/pull/9525#discussion_r394831357
##########
File path: extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureDataSegmentPusher.java
##########
@@ -186,12 +186,24 @@ DataSegment uploadDataSegment(
final DataSegment outSegment = segment
.withSize(size)
- .withLoadSpec(this.makeLoadSpec(new URI(azurePath)))
+ .withLoadSpec(this.makeLoadSpec(segmentConfig.getContainer(), azurePath))
.withBinaryVersion(binaryVersion);
log.debug("Deleting file [%s]", compressedSegmentData);
compressedSegmentData.delete();
return outSegment;
}
+
+ private Map<String, Object> makeLoadSpec(String container, String prefix)
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis merged pull request #9525: Azure deep storage
does not work with datasource name containing non-ASCII chars
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #9525: Azure deep storage does not work with datasource name containing non-ASCII chars
URL: https://github.com/apache/druid/pull/9525
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #9525:
Azure deep storage does not work with datasource name containing non-ASCII
chars
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #9525: Azure deep storage does not work with datasource name containing non-ASCII chars
URL: https://github.com/apache/druid/pull/9525#discussion_r394733243
##########
File path: extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureDataSegmentPusher.java
##########
@@ -186,12 +186,24 @@ DataSegment uploadDataSegment(
final DataSegment outSegment = segment
.withSize(size)
- .withLoadSpec(this.makeLoadSpec(new URI(azurePath)))
+ .withLoadSpec(this.makeLoadSpec(segmentConfig.getContainer(), azurePath))
.withBinaryVersion(binaryVersion);
log.debug("Deleting file [%s]", compressedSegmentData);
compressedSegmentData.delete();
return outSegment;
}
+
+ private Map<String, Object> makeLoadSpec(String container, String prefix)
Review comment:
Could you make the other `makeLoadSpec` delegate to this method instead of duplicating it?
```
@Override
public Map<String, Object> makeLoadSpec(URI uri)
{
return makeLoadSpec(segmentConfig.getContainer(), uri.toString());
}
```
Also, is it necessary to pass `container` in since both invocations are using the same value (if you make the suggested modification)?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org