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

RavenDB-5652 WIP #1265

Closed
wants to merge 2 commits into from
Closed

RavenDB-5652 WIP #1265

wants to merge 2 commits into from

Conversation

myarichuk
Copy link
Contributor

the task is not finished yet, this PR is for purpose of not straying too far from the main branch

@@ -2,7 +2,7 @@
using System.Linq;
using System.Reflection;

[assembly: Raven.Client.RavenVersion(Build = "40", CommitHash = "fd4539d", Version = "4.0", FullVersion = "4.0.0-custom-40")]
[assembly: Raven.Client.RavenVersion(Build = "40", CommitHash = "a0c3e30", Version = "4.0", FullVersion = "4.0.0-custom-40")]
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member

Choose a reason for hiding this comment

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

That is generated by the build.ps1

@@ -24,6 +24,8 @@ public enum ReplyType
public ChangeVectorEntry[] DocumentsChangeVector { get; set; }

public ChangeVectorEntry[] IndexTransformerChangeVector { get; set; }

public string DbId { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

in many places it is called DatabaseId, rename it?

@@ -2,7 +2,7 @@
using System.Linq;
using System.Reflection;

[assembly: Raven.NewClient.RavenVersion(Build = "40", CommitHash = "fd4539d", Version = "4.0", FullVersion = "4.0.0-custom-40")]
[assembly: Raven.NewClient.RavenVersion(Build = "40", CommitHash = "a0c3e30", Version = "4.0", FullVersion = "4.0.0-custom-40")]
Copy link
Member

Choose a reason for hiding this comment

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

?

[DefaultValue(60 * 1000)]
[TimeUnit(TimeUnit.Milliseconds)]
[ConfigurationEntry("Raven/Replication/ReplicationTopologyDiscoveryTimeoutInMs")]
public TimeSetting ReplicationTopologyDiscoveryTimeout { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

Since the value is big, then maybe in Sec?

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 idea


namespace Raven.Server.Documents.Handlers
{
public class FullTopologyHandler : DatabaseRequestHandler
Copy link
Member

Choose a reason for hiding this comment

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

There is a TopologyHandler. Move this endpoint there?

{
public class FullTopologyHandler : DatabaseRequestHandler
{
[RavenAction("/databases/*/full-topology", "GET")]
Copy link
Member

Choose a reason for hiding this comment

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

/topology/graph? /topology/full?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm... yeah this is better

using (var writer = new BlittableJsonTextWriter(context, ResponseBodyStream()))
{
context.Write(writer, new NodeTopologyInfo().ToJson());
writer.Flush();
Copy link
Member

Choose a reason for hiding this comment

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

No need to flush

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perhaps, but what is happening is more apparent with manual flush

using (var writer = new BlittableJsonTextWriter(context, ResponseBodyStream()))
{
context.Write(writer, topology.ToJson());
writer.Flush();
Copy link
Member

Choose a reason for hiding this comment

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

No need to flush

});

writer.Flush();
LastHeartbeatTicks = DateTime.UtcNow.Ticks;
Copy link
Member

Choose a reason for hiding this comment

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

if you have access to DocumentDatabase then use DocumentDatabase.Time.UtcNow, if not then SystemTime.UtcNow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do, but what is the reason for DocumentDatabase.Time.UtcNow?

Copy link
Member

Choose a reason for hiding this comment

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

We are running tests in parallel now, this is why we cannot override global SystemTime, so we introduced a 'local' one for each database

@@ -398,6 +402,7 @@ internal void SendHeartbeat()
private readonly AsyncManualResetEvent _neverSetEvent = new AsyncManualResetEvent();
internal Tuple<ReplicationMessageReply.ReplyType,string> HandleServerResponse()
{
LastHeartbeatTicks = DateTime.UtcNow.Ticks;
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@@ -0,0 +1,49 @@
0 info it worked if it ends with ok
Copy link
Member

Choose a reason for hiding this comment

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

?

new OperationCredentials(null, CredentialCache.DefaultCredentials),
new DocumentConvention())))
{
request.ExecuteRequest();
Copy link
Member

Choose a reason for hiding this comment

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

better use: var json = (RavenJObject)await request.ReadResponseJsonAsync()

using (var clusterTopologyExplorer = new ReplicationClusterTopologyExplorer(
Database,
new Dictionary<string, List<string>>(),
(long)Database.Configuration.Replication.ReplicationTopologyDiscoveryTimeout.AsTimeSpan.TotalMilliseconds,
Copy link
Member

Choose a reason for hiding this comment

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

you are passing long, which is a part of TimeSpan and inside ReplicationClusterTopologyExplorer you are building TimeSpan from that long again, just pass the TimeSpan?

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 idea, don't see a reason why not.

@ml054
Copy link
Member

ml054 commented Jan 3, 2017

You can always rebase from time to time to avoid staying too far from main branch. :)

@ayende ayende closed this Jan 4, 2017
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