Skip to content

Conversation

@Totktonada
Copy link

No description provided.

@Totktonada Totktonada changed the title Support for tarantool clusters (benchmarks) WIP: Fetch list of nodes in a cluster client Mar 6, 2019
Copy link
Author

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

Some random comments.

<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.2</version>
<version>3.6.1</version> <!--incresed from 3.2 due to lack of annotation processing support-->
Copy link
Author

Choose a reason for hiding this comment

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

This comment is more appropriate for a commit message.

/**
* Array of addresses in the form of [host]:[port].
*/
public String[] slaveHosts;
Copy link
Author

Choose a reason for hiding this comment

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

The master-slave terminology is in use for replication, so we should not use it here. I think we can use just 'hosts' (it is addrs—host and port—if we want to be strict) and 'entrypoint'.

*/
public TarantoolClusterClient(TarantoolClusterClientConfig config, String... addrs) {
this(config, new RoundRobinSocketProviderImpl(addrs).setTimeout(config.operationExpiryTimeMillis));
public TarantoolClusterClient(TarantoolClusterClientConfig config) {
Copy link
Author

Choose a reason for hiding this comment

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

Just note for now: here we break backward compatibility w/o a visible reason (nothing prevent us from receiving addrs and save here into a configuration if provided).

* @return the result of SocketChannel open(SocketAddress remote) call
*/
SocketChannel get(int retryNumber, Throwable lastError);
SocketChannel get();
Copy link
Author

Choose a reason for hiding this comment

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

Hmhm. What is the motivation of this change?

import java.util.List;
import java.util.Map;

public class ClusterTopologyFromShardDiscovererImpl implements ClusterTopologyDiscoverer {
Copy link
Author

Choose a reason for hiding this comment

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

We don't discussed any predefined actions around sharding. What is this class and why?

The discussion was about a replication cluster and box.info.replication.

<includeBaseDirectory>false</includeBaseDirectory>
<dependencySets>
<dependencySet>
<outputDirectory>/</outputDirectory>
Copy link
Author

Choose a reason for hiding this comment

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

Why /? Maybe target or testroot is better?

<fileSets>
<fileSet>
<directory>${project.build.directory}/test-classes</directory>
<outputDirectory>/</outputDirectory>
Copy link
Author

Choose a reason for hiding this comment

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

The same question here: why /?

public static final int majorVersion = ${parsedVersion.majorVersion};
public static final int minorVersion = ${parsedVersion.minorVersion};
public static final int majorVersion = Integer.parseInt("${parsedVersion.majorVersion}");
public static final int minorVersion = Integer.parseInt("${parsedVersion.minorVersion}");
Copy link
Author

Choose a reason for hiding this comment

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

Don't add this file into sources manually. IDEA find a generated file when the project is added as maven project. Please, revert this hunk.

@ponomarevDmitri ponomarevDmitri force-pushed the support_for_tarantool_clusters_(benchmarks) branch from 00c2f49 to 65db6ab Compare March 12, 2019 19:23
NUMERIC/DATE-related types were removed and BLOB type
was replaced by SCALAR in scope of new SQL type changes.

See tarantool/tarantool#4019 for more
information.

Fixes tarantool#130.

(cherry picked from commit d2dfa4c)
@Totktonada
Copy link
Author

Superseded by PR #136.

@Totktonada Totktonada closed this Mar 20, 2019
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.

3 participants