Samza-1379: Create Azure Client#254
Conversation
sborya
left a comment
sborya
left a comment
There was a problem hiding this comment.
can you also link the design document to the PR, that explains where this client is needed.
| blobClient = account.createCloudBlobClient(); | ||
| tableClient = account.createCloudTableClient(); | ||
| } catch (IllegalArgumentException | URISyntaxException e) { | ||
| LOG.info("\nConnection string specifies an invalid URI."); |
There was a problem hiding this comment.
It will be a more informative error message if you can include the incorrect value in the message. For example,
LOG.error("Connection string {} specifies an invalid URI", storageConnectionString);
Same suggestion for other error/warning messages.
| tableClient = account.createCloudTableClient(); | ||
| } catch (IllegalArgumentException | URISyntaxException e) { | ||
| LOG.info("\nConnection string specifies an invalid URI."); | ||
| LOG.info("Please confirm the connection string is in the Azure connection string format."); |
| LOG.info("Please confirm the connection string is in the Azure connection string format."); | ||
| throw new SamzaException(e); | ||
| } catch (InvalidKeyException e) { | ||
| LOG.info("\nConnection string specifies an invalid key."); |
| throw new SamzaException(e); | ||
| } catch (InvalidKeyException e) { | ||
| LOG.info("\nConnection string specifies an invalid key."); | ||
| LOG.info("Please confirm the AccountName and AccountKey in the connection string are valid."); |
| compile "com.microsoft.azure:azure-storage:5.3.1" | ||
| compile "com.fasterxml.jackson.core:jackson-core:2.8.8" | ||
| compile project(':samza-api') | ||
| compile project(":samza-core_$scalaVersion") |
There was a problem hiding this comment.
why do we depend on samza-core?
There was a problem hiding this comment.
I think the coordination Apis are in samza-core.
There was a problem hiding this comment.
Samza core has the JobCoordinator, LeaderElector and a few other interfaces that I'm implementing for Azure.
| compile project(':samza-api') | ||
| compile project(":samza-core_$scalaVersion") | ||
| compile "org.slf4j:slf4j-api:$slf4jVersion" | ||
| testCompile "junit:junit:$junitVersion" |
There was a problem hiding this comment.
Will add tests in the future.
build.gradle
Outdated
| } | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Looks good. Thanks for breaking down your changes to smaller PRs :)
One recommendation, given the scope and timeline of your work: Can you add a README.md document in samza-azure module that provide a link to your design doc and other setup related information ? It will be useful for everyone reviewing/using samza-azure.
We discussed that tests can be tabled for later PRs. Please get to it when you can.
Have some comments. Otherwise looks good. 👍
| blobClient = account.createCloudBlobClient(); | ||
| tableClient = account.createCloudTableClient(); | ||
| } catch (IllegalArgumentException | URISyntaxException e) { | ||
| LOG.info("\nConnection string specifies an invalid URI."); |
There was a problem hiding this comment.
It will be a more informative error message if you can include the incorrect value in the message. For example,
LOG.error("Connection string {} specifies an invalid URI", storageConnectionString);
Same suggestion for other error/warning messages.
| import org.apache.samza.config.MapConfig; | ||
|
|
||
|
|
||
| public class AzureConfig extends MapConfig { |
There was a problem hiding this comment.
Can you update the Samza configuration page with a section for "Samza on Azure" and add config relevant to this component?
It helps to add the configs incrementally with your PRs because:
- You can make sure that you don't miss any configs in the end
- It will be useful for reviewers to understand your code better. For example, it's not obvious to me how
AZURE_BLOB_NAMEandAZURE_CONTAINER_NAMEis used and when it should be overridden by the user.
There was a problem hiding this comment.
Updated the configuration page and added a README.md file.
3 participants
@navina
PR 1: AzureClient + AzureConfig (current PR)