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

GORA-667 Revert GORA-271 changes in hbase-store #226

Merged
merged 3 commits into from Oct 30, 2020

Conversation

podorvanova
Copy link
Contributor

This PR still uses DataStoreFactory.getMappingFile() method, when retrieving the filename for the mapping file from the properties file, but also allows to load mappings from properties instead of only from files.

@alfonsonishikawa
Copy link
Member

Hi, @podorvanova , thank you! :D
Something I didn't do at that moment is add a test that verifies that loading from configuration works correctly (my fault). Would you implement a test doing that so will not happen a revert again unnoticed? :)

@podorvanova
Copy link
Contributor Author

@alfonsonishikawa, thank you for your answer!
I will take a look tomorrow.

@alfonsonishikawa
Copy link
Member

@alfonsonishikawa, thank you for your answer!
I will take a look tomorrow.

I see you added the test to MongoDB store in #231

Looks good to me, then. If no one has anything more to comment, I will merge tomorrow.

@@ -887,7 +901,7 @@ private HBaseMapping readMapping(String filename) throws IOException {
LOG.error("Error while trying to read the mapping file {}. "
+ "Expected to be in the classpath "
+ "(ClassLoader#getResource(java.lang.String)).",
filename) ;
mappingStream) ;
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, we should use a different log message, the object mappingStream will not have a human readable content.

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, you are right. I fixed the log message.

@podorvanova
Copy link
Contributor Author

@alfonsonishikawa, I added a test for HBase store as well.

@alfonsonishikawa alfonsonishikawa merged commit c464381 into apache:master Oct 30, 2020
@alfonsonishikawa
Copy link
Member

Merged to master. Thank you, @podorvanova, for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants