Skip to content

Conversation

@IvanKolchanov
Copy link
Contributor

@IvanKolchanov IvanKolchanov commented Jul 15, 2024

Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

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

Left some suggestions

Task<QueryResult<CoordinateEntry[]>> Node(string node, QueryOptions q, CancellationToken ct = default);
Task<QueryResult<CoordinateEntry[]>> Node(string node, CancellationToken ct = default);
Task<QueryResult<CoordinateEntry[]>> Nodes(QueryOptions q, CancellationToken ct = default);
Task<WriteResult> Update(CoordinateEntry entry, CancellationToken ct = default);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we also need a variant with WriteOptions

Assert.Equal("OK", response.StatusCode.ToString());

var newCoordResult = await _client.Coordinate.Node(nodeName);
for (int i = 0; i < 5; i++)
Copy link
Contributor

@marcin-krystianc marcin-krystianc Jul 16, 2024

Choose a reason for hiding this comment

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

I'm not a fan of that loop.
I think there is a reliable way to query the node without using the loop.
You can use so called "blocking queries", i.e.: use query options with a non-zero wait-index:

   await _client.Catalog.Register(registration);
   var nodeResult = await _client.Catalog.Node(nodeName);
   Assert.Equal(HttpStatusCode.OK, nodeResult.StatusCode);
...
   await _client.Coordinate.Update(coord);
...
   var q = new QueryOptions{ WaitIndex = nodeResult.LastIndex, };
   var newCoordResult = await _client.Coordinate.Node(nodeName, q);
   Assert.Equal(HttpStatusCode.OK, response.StatusCode);

I tried this locally and it seemed working reliably.

Copy link
Contributor Author

@IvanKolchanov IvanKolchanov Jul 16, 2024

Choose a reason for hiding this comment

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

That's fair, there wasn't really a similar situation in the previous code, so I went for something similar to Golang code's retry loop. Should we change waitTime to around 10 seconds, to avoid hanging for the default 5 minutes? And I have a little question, I am a bit confused on how is the index of node registered in catalog connected to the coordinate node? I am still working on my familiarity with all consul features, sorry.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries, it is sometimes a bit of unknown for me as well 😄 I'm concerned about the loop because it introduces a risk of flakiness. We run each test many times (we have a matrix with os, consul version and runtime version), thus we need to have really robust tests.
I think the "LastIndex"/"WaitIndex" are global, so it doesn't matter that they are from different endpoints.

Should we change waitTime to around 10 seconds, to avoid hanging for the default 5 minutes
I think it is better to wait longer, but have more robust tests. Therefore I wouldn't change the default waitTime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!) Makes sense, I will commit all the changes and we see how the tests go.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: I've just updated my code snipped from above, to make it clear that the Catalog.Node() call should be before the Coordinate.Update() call.

@IvanKolchanov
Copy link
Contributor Author

@marcin-krystianc

Left some suggestions

Okey, everything should be good

@marcin-krystianc marcin-krystianc marked this pull request as ready for review July 16, 2024 12:23
Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

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

Nice 👍

@marcin-krystianc
Copy link
Contributor

PS: I think you need to push some dummy changes, so the last commit is not from whitespace fixing bot. Otherwise, It is impossible to merge it.

@marcin-krystianc marcin-krystianc merged commit cffe268 into G-Research:master Jul 16, 2024
@IvanKolchanov IvanKolchanov deleted the add-update-lan-coordinates branch July 17, 2024 14:02
@IvanKolchanov IvanKolchanov changed the title Added GET Update LAN Coordinates endpoints, added tests for it Added PUT Update LAN Coordinates endpoints, added tests for it Aug 2, 2024
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.

FEATURE REQUEST: Update LAN Coordinates for a node

3 participants