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

New Feature: Configuration Settings #33

Open
9 of 11 tasks
MichaCo opened this issue Aug 19, 2018 · 1 comment
Open
9 of 11 tasks

New Feature: Configuration Settings #33

MichaCo opened this issue Aug 19, 2018 · 1 comment

Comments

@MichaCo
Copy link
Owner

MichaCo commented Aug 19, 2018

All this is subject to change/work in progress

  • Options/Settings API rework
    • First version of the changes (6f2d948)
    • Finalize
  • Rework "by server" query to configured/custom query
    • Replace "by server" queries with optional DnsQueryOptions
      • Revert that change, maybe simplify some overloads, but that API might still be useful => Configure the client once, use it with different servers. (The initial configuration doesn't require servers)
    • Decide on fallback of name servers (e.g. if nothing is specified in query options, use the client's name servers)
  • Breaking changes marked as obsolete instead
  • More unit tests to validate the different ways of configuring a query
  • Remove obsolete parts in the future (v1.x/2.0)
  • Documentation changes in all examples and on the website

The Problem

The LookupClient class has many properties which define the behavior of that class, like number of retries for a query, timeouts, caching etc...
All properties can be set at any point in time because LookupClient is not immutable.
Also, LookupClient is not a cheap object and it is advisable to reuse it as much as possible.

The problem with this is, that the state of LookupClient can be altered by different parts of an application and can cause unpredictable behavior.

For example, 2 different threads run a query with LookupClient at the same time. One sets UseCaching to true and one to false, before running the query.
There is no way to ensure thread safety at this point for the UseCaching property to be set correctly.

Solution

I will remove all properties from the LookupClient itself and move them to a options class which can be used to initialize LookupClient.

previous version

var client = new LookupClient(NameServer.GooglePublicDns)
{
    UseCache = true,
    EnableAuditTrail = false,
    Retries = 3,
    Timeout = TimeSpan.FromSeconds(10),
    ThrowDnsErrors = true
};

new version

var client = new LookupClient(
    new LookupClientOptions(NameServer.GooglePublicDns)
    {
        UseCache = true,
        EnableAuditTrail = false,
        Retries = 3,
        Timeout = TimeSpan.FromSeconds(10),
        ThrowDnsErrors = true                   
    });

LookupClientOptions can be changed as much as needed before it gets passed to the client's ctor (which also give a little bit more flexibility to consumers).

After the LookupClient gets initialized, the options are stored as read-only copy and cannot be changed anymore.

The settings can be accessed via client.Settings.

This is a Breaking Change

Yes, this will be a bigger breaking change, because all the properties on the class are getting removed and the way to configure LookupClient changes.

Instead of removing the existing functionality right away, I will mark everything obsolete which will be removed some versions later (probably in 2.0).

Additional changes:

The list of name servers was a collection of IPEndPoints. IPEndPoint itself is mutable though, so it would be possible to change e.g. the IP address of an endpoint.
That will be changed, too. Instead, the NameServer class becomes an immutable type and will be used across the board.

Custom Queries

In version 1.1.0, I added "by server" query overloads which allows to query a different endpoint than configured initially.
Those endpoints will be removed alltogether in the next version (yes its breaking but I hope that feature wasn't really used much yet).
Instead, there will be new Query... methods taking a subset of LookupClientOptions as an additional/optional parameter.

There will be new overloads to Query, QueryAsync and QueryReverse which take DnsQueryOptions, which is a subset of LookupClientOptions.

With that, one can fully configure individual queries while still using the same client instance.
This feature can be used to query a dedicated endpoint or just use a different configuration for a particular query.

Example A, disabling cache for one query.

client.Query("google.com", QueryType.A, queryOptions: new DnsQueryOptions(NameServer.GooglePublicDns)
{
    UseCache = false
});

The query options will replace any settings previously configured on the client. There is no fallback to what was configured previously (including name servers)!, except for name servers.
In case the passed in query options do not provide one or more name servers, the query will try to use the name servers of the LookupClient instance.

If anyone did read all of this ;) and has comments/ideas, let me know; happy to get feedback on this.

@MichaCo MichaCo added this to the 1.3.0 milestone Aug 19, 2018
@MichaCo MichaCo self-assigned this Aug 19, 2018
MichaCo added a commit that referenced this issue Aug 19, 2018
… server was defined (seems reasonable)

Added used settings property to the returned result
#33
MichaCo added a commit that referenced this issue Aug 20, 2018
MichaCo added a commit that referenced this issue Aug 20, 2018
Because servers can be configured at execution time, per query. #33
First pass at unit testing all the configuration options and combinations...
@MichaCo
Copy link
Owner Author

MichaCo commented Mar 5, 2020

:update: Development here is mostly complete.
It will be a non-breaking change in 1.3.0! see #57

MichaCo added a commit that referenced this issue Mar 16, 2020
* Making name server collection in `DnsQueryAndServerOptions` readonly
* Changing the way Options and Settings work slightly. Resolve name servers now happens in LookupClient's ctor instead of the options. See #33 
   * New property on `AutoResolveNameServers` which defaults to `True` if no name servers are passed in. Goes to `False` if there are (but can be set to true again...
* IDnsQuery API's with options simplified and I went back from using optional parameters, there are 2 overloads now instead, but not for all variants.
@MichaCo MichaCo changed the title Rework of all LookupClient's properties (tracking issue) New Feature: Configuration Settings Mar 17, 2020
Repository owner locked as resolved and limited conversation to collaborators Mar 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant