Skip to content
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

Add support for Aliyun Object Storage Service (OSS) #485

Closed
wants to merge 1 commit into from

Conversation

frankgh
Copy link
Contributor

@frankgh frankgh commented Nov 9, 2020

Add dependencies to support Aliyun OSS and add automation testing for
the HCFS group. Add oss-site.xml template to configure aliyun cloud.

Add dependencies to support Aliyun OSS and add automation testing for
the HCFS group. Add oss-site.xml template to configure aliyun cloud.
@lisakowen
Copy link
Contributor

just wondering about the template file name, oss-site.xml. would it make sense to qualify the name somehow? when i see oss, i think of open source software.

@frankgh
Copy link
Contributor Author

frankgh commented Nov 10, 2020

just wondering about the template file name, oss-site.xml. would it make sense to qualify the name somehow? when i see oss, i think of open source software.

@lisakowen that's what the service is called. We can rename it to something that makes more sense though. I'm not sure if this PR will be a priority. Let me check on that first

@@ -211,6 +211,16 @@ ifneq "$(PROTOCOL)" ""
sed $(SED_OPTS) "s|YOUR_AZURE_BLOB_STORAGE_ACCOUNT_NAME|$(WASB_ACCOUNT_NAME)|" $(PROTOCOL_HOME)/$(PROTOCOL)-site.xml; \
sed $(SED_OPTS) "s|YOUR_AZURE_BLOB_STORAGE_ACCOUNT_KEY|$(WASB_ACCOUNT_KEY)|" $(PROTOCOL_HOME)/$(PROTOCOL)-site.xml; \
fi; \
if [ $(PROTOCOL) = oss ]; then \
if [ -z "$(OSS_ACCESS_KEY_ID)" ] || [ -z "$(OSS_SECRET_ACCESS_KEY)" ] || [ -z "$(OSS_ENDPOINT)" ]; then \
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if these are following an existing convention or if they have to be named this way because third-party deps require them, but the OSS_* seems very generic. Could we prepend ALIYUN_ to the environment variables?

rm -rf $(PROTOCOL_HOME); \
exit 1; \
fi; \
sed $(SED_OPTS) "s|YOUR_OSS_ACCESS_KEY_ID|$(OSS_ACCESS_KEY_ID)|" $(PROTOCOL_HOME)/$(PROTOCOL)-site.xml; \
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these calls to sed supposed to be editing in place? Are you missing -i?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@@ -16,6 +16,7 @@ public String getExternalTablePath(String basePath, String path) {
return StringUtils.removeStart(path, basePath);
}
},
OSS("oss"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Inline with my comment above, is it possible to make this aliyun-oss?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I think we need a good name for aliyun's service. OSS is what their service is called ... but it's pretty confusing>.

</dependency>

<!-- Use version 3.8.1 (version 3.0.0 would produce a lot of output for
a NoSuchKey error. The issue is detailed here:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would we be hitting the NoSuchKey error?

@@ -367,7 +367,7 @@ public void textFormatBZip2CopyFromStdin() throws Exception {
*
* @throws Exception if test fails to run
*/
@Test(groups = {"features", "gpdb", "hcfs", "security"}, timeOut = 120000)
@Test(groups = {"features", "gpdb", "hcfs", "security"}, timeOut = 180000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we expect the newer aliyun tests to run slower? Why did the timeout need to be increased?

<profile>
<name>oss:avro</name>
<description>This profile is suitable for using when reading Avro files (i.e
fileName.avro)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fileName.avro)
fileName.avro) from Alibaba Cloud

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems like you specify Alibaba Cloud in all the other profiles, so we should keep this consistent across all the profiles.

<profile>
<name>oss:json</name>
<description>
Access JSON data either as:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Access JSON data either as:
Access JSON data from Alibaba Cloud either as:

<profile>
<name>oss:SequenceFile</name>
<description>
Profile for accessing Sequence files serialized with a custom Writable class
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Profile for accessing Sequence files serialized with a custom Writable class
Profile for accessing Sequence files serialized with a custom Writable class from Alibaba Cloud

@frankgh frankgh marked this pull request as draft December 9, 2020 16:57
@frankgh
Copy link
Contributor Author

frankgh commented Feb 19, 2021

Closing until we have requests for Aliyun Object Storage Service support

@frankgh frankgh closed this Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants