Opened 7 years ago

Closed 7 years ago

#2852 closed task (fixed)

Add an API for (re)setting a memory segment to ConfigurableClientList

Reported by: jinmei Owned by: muks
Priority: medium Milestone: Sprint-20130611
Component: data source 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: 3 Add Hours to Ticket: 0
Total Hours: 1.98 Internal?: no

Description (last modified by jinmei)

Subtask of #2830. Depend on #2850.

It would look like this:

// params: opaque parameter specific to the memory segment type
// in case of "mapped" segment, it would be something like:
// {"mapped-file": "/var/bind10/mapped-files/zone-sqlite3.mapped.0"}
void
ConfigurableClientList::resetMemorySegment(const string& datasrc_name,
                                           MemorySegmentMode mode,
                                           ConstElementPtr params);

It would simply call the corresponding ZoneTableSegment::reset().

For mode parameter, see #2850.

Also add python wrapper of this method.

Subtickets

Change History (17)

comment:1 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 7 years ago by jinmei

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

comment:3 Changed 7 years ago by jinmei

  • Milestone changed from Previous-Sprint-Proposed to Next-Sprint-Proposed
  • Priority changed from medium to high

comment:4 Changed 7 years ago by muks

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

comment:5 Changed 7 years ago by jinmei

  • Priority changed from high to medium

comment:6 Changed 7 years ago by muks

  • Owner set to muks
  • Status changed from new to assigned

Picking

comment:7 Changed 7 years ago by muks

  • Owner changed from muks to UnAssigned
  • Status changed from assigned to reviewing

I'm putting this to review (after discussion with Shane). Tests for the Python bindings require ZoneWriter in #2853 to be completed. The rest of the work can be reviewed.

The tests for the Python bindings will be added as a part of #2853.

comment:8 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:9 Changed 7 years ago by vorner

  • Owner changed from vorner to muks

Hello

I think the C++ tests are missing too. Should they be present? There may be a comment about when the python tests will appear.

What happens if the name of data source is wrong? It seems it'll just silently do nothing. Should it report the error (that such data source does not exist) somehow instead?

The python documentation says it is a wrapper. While this is true, I'm not sure if it is something to bother the user of the API with, or if it's just a small implementation detail.

comment:10 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to vorner

Hi vorner

Sorry I forgot to mention the name of the branch. It is trac2852_3. This should modify the C++ test code so that the ListTest.* tests run using a mapped file.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to muks

Hello

Replying to muks:

Sorry I forgot to mention the name of the branch. It is trac2852_3. This should modify the C++ test code so that the ListTest.* tests run using a mapped file.

That looks as a reasonable test.

However, should the test case be parametrized, so it works both with the file-mapped segment and with the memory one? To check that such change as resetting the segment doesn't cause the memory one stop working?

Also, where does the config that is passed to the reset method come from?

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

Replying to vorner:

However, should the test case be parametrized, so it works both with the file-mapped segment and with the memory one? To check that such change as resetting the segment doesn't cause the memory one stop working?

I have parameterized the tests now. It also is useful to do this as shared memory is not available on all platforms. Jinmei mentioned that it is an invalid operation to call reset() on a ZoneTableSegmentLocal (which is why it throws), but I don't know why it has to be like this instead of being a nop.

Also, where does the config that is passed to the reset method come from?

Somewhere in the B10 config, but I don't know which ticket specifies this. ZoneTableSegmentMapped expects a map element with this specific key/value pair.

comment:13 Changed 7 years ago by muks

We should have specified this in #2832 clearly. I guess this comes from the value of "params" key.

comment:14 Changed 7 years ago by muks

  • Owner changed from muks to vorner

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

  • Owner changed from vorner to muks
  • Total Hours changed from 0 to 1.98

Hello

I think it is OK to merge now.

Though it is kind of bad that neither of us know the reasons for how it should be this way and such. It makes me worry.

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

Replying to vorner:

Though it is kind of bad that neither of us know the reasons for how it should be this way and such. It makes me worry.

About what? Whether to throw or ignore it if reset() is called for
a non writable segment?

comment:17 Changed 7 years ago by muks

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

Merged to master in commit d97165b66927e4a58979db9558e2b2f37fea17e9:

* ab78760 [2852] Parameterize ListTest.* and run it with both local and memory segments
* b692530 [2852] Rename local variable (don't use member variable syntax)
* e58e863 [2852] Delete DataSourceClient objects before clearing the vector
* 425ee82 [2852] Move object to inner block
* 66564c1 [2852] Use a mapped memory segment in the ListTests
* 47a2fe2 [2852] Remove unused variable
* 572c201 [2852] Add API for resetting a memory segment to ConfigurableClientList

Resolving as fixed. Thank you for the reviews Michal.

Note: See TracTickets for help on using tickets.