Opened 7 years ago

Closed 7 years ago

#2832 closed task (fixed)

extend data source config to specify shmem segment params

Reported by: jinmei Owned by: pselkirk
Priority: medium Milestone: Sprint-20130423
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: shared memory data source
Estimated Difficulty: 2 Add Hours to Ticket: 0
Total Hours: 0.75 Internal?: no

Description (last modified by jinmei)

subtask of #2830. no dependency.

We'll extend the data source configuration so we can specify the type
of cache (in memory) memory segment type and its parameter, if any.
It would look like this:

"IN": [{
    "type": "sqlite3",
    "name": "sqlite3",
    "params": {"database_file": "/var/bind10/zone.sqlite3"},
    "cache-zones": ["example.org", "example.com"],
    "cache-enable": true,
    "cache-segment-type": "mapped"
}]

The default value of the "name" is the data source type (as that would
usually be unique). If there are more than one instances of the
same data source type, the administrator is responsible for giving
them unique names; the configuration checker should validate the uniqueness.

cache-segment-type has the default of "local".

For now we only extend the spec. It will be simply ignored if
specified in the actual config.

Subtickets

Attachments (1)

cache-config.diff (1.5 KB) - added by jinmei 7 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 follow-up: Changed 7 years ago by vorner

Should the directory be on per-zone basis? Shouldn't that be better a global memmgr setting?

comment:2 in reply to: ↑ 1 ; follow-up: Changed 7 years ago by jinmei

Replying to vorner:

Should the directory be on per-zone basis?

Assuming we are going to use a single mapped file for all zones, no.

Shouldn't that be better a global memmgr setting?

Where to specify this information is actually something I'm not sure
yet, but I'm also not sure what the "global" memmgr setting is. Can
you show an example?

After thinking about this a bit closely, I'm currently feeling that it
makes sense to simply specify a file (with postfix for versioning)
just like the way we specify the sqlite3 DB file. But as I said
this is still a bit open.

comment:3 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:4 in reply to: ↑ 2 Changed 7 years ago by vorner

Hello

Replying to jinmei:

Shouldn't that be better a global memmgr setting?

Where to specify this information is actually something I'm not sure
yet, but I'm also not sure what the "global" memmgr setting is. Can
you show an example?

Well, outside of the data_sources pseudo-module. Something like
/MemMgr/storage "/path/to/directory". It would default to something near the
place we store config and sqlite databases. Then it would simply create files
as it needs there and tell the other modules in which file it'll find which
zone.

Because, in your case, what happens if you have one data source with one
setting and other with different? Does it choose one? Does it create a separate
file for each of the data sources? If so, will it be able to comply with
setting both data sources to the same value correctly?

After thinking about this a bit closely, I'm currently feeling that it
makes sense to simply specify a file (with postfix for versioning)
just like the way we specify the sqlite3 DB file. But as I said
this is still a bit open.

If there's more than one file (like with the versioning), I think it is more
intuitive to use directory. If I set a filename, I expect that file to exist
and not other with some suffix, or multiple files. If I set directory, I expect
the program to do whatever it likes in the directory.

comment:5 Changed 7 years ago by jinmei

  • Description modified (diff)
  • Milestone changed from Previous-Sprint-Proposed to Next-Sprint-Proposed

(I've revised the description based on the latest design).

comment:6 follow-up: Changed 7 years ago by vorner

Just a small suggestion. The user probably should not be required to know about memory segments, that's our internal representation. What about "cache-type": "SharedFile"?

The name is already being added in #2835.

comment:7 in reply to: ↑ 6 Changed 7 years ago by jinmei

Replying to vorner:

Just a small suggestion. The user probably should not be required to know about memory segments, that's our internal representation. What about "cache-type": "SharedFile"?

The name is already being added in #2835.

The main reason for specifying the type of the memory segment is that
we might eventually want to other type of shared segment, such as
non-file based one or Windows-specific one. And, at least the file
based segment is special in that we need to create and maintain files
somewhere, so some kind of path information would have to be
configured (even if it has some default and can be omitted). If
there's a better abstraction that meets such requirement and still
hides lower level details, I'm okay with that.

comment:8 Changed 7 years ago by vorner

I don't say we should not configure them. I'm just saying that „Segment“ is our internal development name in the code and we don't need users asking what the segments are.

The proposal is to call it „cache-type“ instead of „cache-segment-type“.

comment:9 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130402

comment:10 Changed 7 years ago by pselkirk

  • Owner set to pselkirk
  • Status changed from new to accepted

comment:11 follow-up: Changed 7 years ago by pselkirk

  • Owner changed from pselkirk to UnAssigned
  • Status changed from accepted to reviewing

Added support for 'cache-type' (using vorner's terminology) in datasrc.spec.

Does this need a lettuce test or anything else at this point?

comment:12 in reply to: ↑ 11 Changed 7 years ago by jinmei

Replying to pselkirk:

Added support for 'cache-type' (using vorner's terminology) in datasrc.spec.

Does this need a lettuce test or anything else at this point?

I'd add a few trivial unittests: explicitly setting cache-type to
"local" and see it doesn't cause disruption, setting it to "something
bogus" and confirm it's rejected (I believe it's rejected).

Also, as I suggested before, I'd have suggested explicitly mentioning
whether we need a changelog entry for this task in the ticket log
(even if it's deemed too obvious to mention, until you are
sufficiently familiarized with the practice). And, in this case, it
might not be that obvious even if the change is very small, because
it extends the user interface. But, in the end, I would personally
skip a changelog at this point - until we actually support other types
of cache-type it's almost meaningless.

comment:13 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to pselkirk

comment:14 follow-up: Changed 7 years ago by pselkirk

  • Owner changed from pselkirk to jinmei

I added value checking in getSegmentTypeFromConf, so that it only accepts "local" and "mapped".
Also added a unittest case that tries to set it to "bogus".

I agree that it doesn't need a changelog entry, because there's nothing that actually uses it yet, so no reason for the user to try to set it.

comment:15 in reply to: ↑ 14 Changed 7 years ago by jinmei

Replying to pselkirk:

I added value checking in getSegmentTypeFromConf, so that it only accepts "local" and "mapped".
Also added a unittest case that tries to set it to "bogus".

Hmm, I actually intended to test it for
src/bin/cfgmgr/plugins/datasrc_config_plugin.py
but I now realized it wouldn't work as its checker disables cache.

Also, my intent was to have CacheConfig treat cache-type opaque
string and validate the type only in ZoneTableSegment::create().
And, "mapped" type would probably be optional partly because it causes
some unsolvable portability issue (at least in near time).

Within the C++ code we already test ZoneTableSegment::create()
reject a very unlikely name and confirm "cache-type" config value will
be returned by getSegmentType(), so I now think we don't need any
new test to this end.

The typo fix in 946b1dfec880e605bc154bc963f4b3bd860c1b55 looks
correct, so I suggest only leaving it and committing it without the
additional check and test.

I'm attaching a diff to do this. If you agree, please apply it and
merge.

Changed 7 years ago by jinmei

comment:16 Changed 7 years ago by jinmei

  • Owner changed from jinmei to pselkirk

comment:17 Changed 7 years ago by jinmei

  • Total Hours changed from 0 to 0.75

comment:18 Changed 7 years ago by pselkirk

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged to master branch in commit f85cdd324c2d51046c2030587d8d5053b4a0ebbd:

* 754c38e [2832] roll back all of 946b1df except typo fix
* 946b1df [2832] add value checking for 'cache-type'
* 9a160a1 [2832] add datasrc spec support for 'cache-type'
Note: See TracTickets for help on using tickets.