-
Notifications
You must be signed in to change notification settings - Fork 278
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
S3 Implementations of Connector Interfaces #46
S3 Implementations of Connector Interfaces #46
Conversation
ajoymajumdar
commented
Feb 13, 2017
- Removed metacat-convertersmodule.
- Removed the depedncies on presto.
- Refactored code. Moved code to metacat-common-server. Also disabled certain modules. These modules will be replaced by new ones.
- Added s3 connector for the new connector interfaces.
- Made relevant changes in metacat-main and metacat-thrift.
… depedencies on presto libraries. 1. Removed metacat-convertersmodule. 2. Removed the depedncies on presto. 3. Refactored code. Moved code to metacat-common-server. Also disabled certain modules. These modules will be replaced by new ones. 4. Added s3 connector for the new connector interfaces. 5. Made relevant changes in metacat-main and metacat-thrift.
public class ConnectorContext { | ||
private final long timestamp; | ||
private final String userName; | ||
private long timestamp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason this is now mutable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dozer was not able to map MetacatRequestContext to ConnectorContext if it does not have an default empty constructor. We can define factories within dozer to overcome the problem.
* @return connector factory | ||
*/ | ||
ConnectorFactory create(); | ||
ConnectorFactory create(String connectorName, Map<String, String> configuration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can these be final
and are they nullable?
|
||
@Override | ||
public ConnectorTypeConverter get() { | ||
final MetacatRequestContext requestContext = MetacatContextManager.getContext(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This MetacatContextManager is something thread local? No way for the context to get mixed up between requests right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use fork of the request to multiple threads, then you will lose the context unless you set it in each of the child threads. It should be okay if you do not fork it.
* @param config config | ||
*/ | ||
@Inject | ||
public TypeConverterProvider( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a delta between when this object is constructed and when others are provided. What is in place to enforce that the system is fully setup before get() can start being called? We don't want to throw the IllegalArgumentExceptions just cause this isn't fully boostrapped. Maybe the map of String, TypeConverter can be injected at construction time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will you inject the available plugin's type converter to the provider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we do a classpath scan for all implementations of the type converter? Also expose a method within them for some kind of indentifier
} | ||
|
||
@Override | ||
public ConnectorInfoConverter getInfoConverter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should be providing default implementations of these interfaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point.
@Override | ||
public DatabaseInfo get(@Nonnull final ConnectorContext context, @Nonnull final QualifiedName name) { | ||
final String databaseName = name.getDatabaseName(); | ||
Preconditions.checkNotNull(databaseName, "Schema name is null"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we agree to name everything database
shouldn't this say database not schema?
location.setInfo(newInfo); | ||
newInfo.setLocation(location); | ||
} else { | ||
if (newInfo.getInputFormat() != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these checks for null will be good places to replace with Optional
* @param configuration configuration properties | ||
* @param infoConverter S3 info converter | ||
*/ | ||
public S3Module(final String catalogName, final Map<String, String> configuration, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why pass in the info converter? isn't that going to be consistent across all instantiations of this module? I'd just build it in here.
* The base dao. | ||
* @param <T> | ||
*/ | ||
public interface BaseDao<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spring Data JPA.... just sayin'
* limitations under the License. | ||
*/ | ||
|
||
package com.netflix.metacat.connector.s3.model; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the package be entities
1. Removed metacat-convertersmodule. 2. Removed the depedncies on presto. 3. Refactored code. Moved code to metacat-common-server. Also disabled certain modules. These modules will be replaced by new ones. 4. Added s3 connector for the new connector interfaces. 5. Made relevant changes in metacat-main and metacat-thrift.