Skip to content

Azure deep storage does not work with datasource name containing non-ASCII chars#9525

Merged
clintropolis merged 4 commits intoapache:masterfrom
zachjsh:IMPLY-2441
Mar 19, 2020
Merged

Azure deep storage does not work with datasource name containing non-ASCII chars#9525
clintropolis merged 4 commits intoapache:masterfrom
zachjsh:IMPLY-2441

Conversation

@zachjsh
Copy link
Copy Markdown
Contributor

@zachjsh zachjsh commented Mar 17, 2020

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.


This PR has:

  • been self-reviewed.
  • 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
  • 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.

…ASCII chars

Fixed a bug where recording the segment file location fails when
using Azure Deep Storage, if the datasource has any special
characters
@maytasm
Copy link
Copy Markdown
Contributor

maytasm commented Mar 17, 2020

If this PR #9501 goes in, you can also run the integration test for Azure which will verify your fix.

return outSegment;
}

private Map<String, Object> makeLoadSpec(String container, String prefix)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

"blobPath",
uri.toString()
);
return makeLoadSpec(uri.toString());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@clintropolis clintropolis merged commit 4870ad7 into apache:master Mar 19, 2020
@zachjsh zachjsh deleted the IMPLY-2441 branch March 19, 2020 20:41
@jihoonson jihoonson added this to the 0.18.0 milestone Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Azure deep storage does not work with datasource name containing non-ASCII chars

4 participants