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

S3 Implementations of Connector Interfaces #46

Merged
merged 9 commits into from
Feb 15, 2017

Conversation

ajoymajumdar
Copy link
Contributor

  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.

… 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.
@tgianos tgianos changed the title This commit works with the new connector interface for s3 catalog. S3 Implementations of Connector Interfaces Feb 13, 2017
public class ConnectorContext {
private final long timestamp;
private final String userName;
private long timestamp;
Copy link
Contributor

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?

Copy link
Contributor Author

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);
Copy link
Contributor

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();
Copy link
Contributor

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?

Copy link
Contributor Author

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(
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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() {
Copy link
Contributor

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?

Copy link
Contributor Author

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");
Copy link
Contributor

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) {
Copy link
Contributor

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,
Copy link
Contributor

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> {
Copy link
Contributor

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;
Copy link
Contributor

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

@ajoymajumdar ajoymajumdar merged commit 859dbf4 into Netflix:canonical-types Feb 15, 2017
ajoymajumdar added a commit that referenced this pull request May 11, 2017
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.
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

2 participants