Hi Vu, ACK. not tested.
-AVM On 4/28/2017 1:27 PM, Vu Minh Nguyen wrote: > Hi Mahesh, > > Regarding your suggestion, I think we should do it in a separated ticket > that is re-organize headers/implementation files. > > How do you think? > > Regards, Vu > >> -----Original Message----- >> From: A V Mahesh [mailto:[email protected]] >> Sent: Friday, April 28, 2017 4:56 AM >> To: Canh Van Truong <[email protected]>; >> [email protected]; [email protected] >> Cc: [email protected] >> Subject: Re: [PATCH 1/1] log: fix checkpoint dest_names in open stream >> request [#2434] >> >> Hi Canh, >> >> On 4/27/2017 5:14 PM, Canh Van Truong wrote: >>> As current design, the configuration stream is just deleted (e.g. immcfg -d >> <STREAM_DN>) >> I did see that , near future we my need to associated client when >> deleting the stream >> the that is way my comment was irrelevant of clientId used or not for >> stream_close , change this >> `ckpt_stream_close(log_stream_t *stream, time_t closetime)` >> >> On 4/27/2017 5:14 PM, Canh Van Truong wrote: >>> Handling of checkpoint for stream close is in several places. Maybe we >> could refactor to handle in one place to make clean code. >> >> At lease for code readability , In this patch at lease we need to move >> `ckpt_stream_close(log_stream_t *stream, time_t closetime)` same file >> where lgs_ckpt_stream_open() located. >> >> -AVM >> >> On 4/27/2017 5:14 PM, Canh Van Truong wrote: >>> Hi Mahesh, >>> >>> Handling of checkpoint for stream close is in several places. Maybe we >> could refactor to handle in one place to make clean code. >>> But it is not related to this ticket. We could refactor it in later ? >>> >>> Regards >>> Canh >>> >>> -----Original Message----- >>> From: Canh Van Truong [mailto:[email protected]] >>> Sent: Thursday, April 27, 2017 1:27 PM >>> To: 'A V Mahesh' <[email protected]>; >> '[email protected]' <[email protected]>; >> '[email protected]' <[email protected]> >>> Cc: '[email protected]' <opensaf- >> [email protected]> >>> Subject: RE: [PATCH 1/1] log: fix checkpoint dest_names in open stream >> request [#2434] >>> Hi Mahesh, >>> >>> As current design, the configuration stream is just deleted (e.g. immcfg -d >> <STREAM_DN>) if the stream is opened only one time at creating cfg stream >> (stream->numOpeners = 1). You can check at >> "stream_ccb_completed_delete" function >>> If stream->numOpeners > 1, the activity deleting configuration stream is >> failed. >>> It means that the configuration stream is just deleted when no client is >> associated with this stream. So the client_id is assigned -1 when checkpoint >> at stream close in deleting configuration stream. >>> An invalid client_id is updated here for checkpoint data. >>> >>> >>> In "proc_stream_close_msg", the valid client_id was updated for >> checkpoint data when check-pointing. >>> Thanks >>> Canh >>> >>> >>> >>> -----Original Message----- >>> From: A V Mahesh [mailto:[email protected]] >>> Sent: Thursday, April 27, 2017 10:35 AM >>> To: Canh Van Truong <[email protected]>; >> [email protected]; [email protected] >>> Cc: [email protected] >>> Subject: Re: [PATCH 1/1] log: fix checkpoint dest_names in open stream >> request [#2434] >>> Hi Canh, >>> >>> On 4/27/2017 12:34 PM, Canh Van Truong wrote: >>>> For checkpoint stream open in proc_stream_open_msg, lgs need update a >>>> valid client_id for checkpoint data >>> I was asking, is For checkpoint stream close in proc_stream_close_msg, >>> lgs need update a valid client_id for checkpoint data ? >>> in `ckpt_stream_close` client_id is checkpoint as -1 now. >>> >>> irrelevant of clientId used or not for stream_close , change this >>> `ckpt_stream_close(log_stream_t *stream, time_t closetime)` >>> function also to ckpt_stream_close(log_stream_t *stream, time_t >>> closetime, const uint32_t &client_id); >>> >>> >> ============================================================== >> ====================== >>> static uint32_t ckpt_stream_close(log_stream_t *stream, time_t >> closetime) { >>> uint32_t rc; >>> lgsv_ckpt_msg_v1_t ckpt_v1; >>> lgsv_ckpt_msg_v2_t ckpt_v2; >>> void *ckpt_ptr; >>> >>> TRACE_ENTER(); >>> >>> if (lgs_is_peer_v2()) { >>> memset(&ckpt_v2, 0, sizeof(ckpt_v2)); >>> ckpt_v2.header.ckpt_rec_type = LGS_CKPT_CLOSE_STREAM; >>> ckpt_v2.header.num_ckpt_records = 1; >>> ckpt_v2.header.data_len = 1; >>> >>> /* No client. Logservice itself has opened stream */ >>> ckpt_v2.ckpt_rec.stream_close.clientId = -1; >>> >>> >> ============================================================== >> ====================== >>> -AVM >>> >>> On 4/27/2017 12:34 PM, Canh Van Truong wrote: >>>> Hi Mahesh, >>>> >>>> For checkpoint stream open in proc_stream_open_msg, lgs need update a >>>> valid client_id for checkpoint data >>>> >>>> For checkpoint stream open in create configuration stream (in >>>> stream_ccb_apply_create) and in encode the cold sync >>>> (edu_enc_streams), lgs update an invalid client_id (-1) for checkpoint >>>> data. Because the client_id is not needed here for creating >>>> configuration stream when no client is associated with the stream. The >>>> cold sync is the same reason. >>>> >>>> I am not sure if I understand your question? Could you clear it to me? >>>> >>>> Thanks >>>> >>>> Canh >>>> >>>> -----Original Message----- >>>> From: A V Mahesh [mailto:[email protected]] >>>> Sent: Thursday, April 27, 2017 8:20 AM >>>> To: Canh Van Truong <[email protected]>; >>>> [email protected]; [email protected] >>>> Cc: [email protected] >>>> Subject: Re: [PATCH 1/1] log: fix checkpoint dest_names in open stream >>>> request [#2434] >>>> >>>> Hi Canh Van, >>>> >>>> As if you are updating client_id while lgs_ckpt_stream_open() checkpoint >>>> >>>> is client_id not required as reference to while ckpt_stream_close() >>>> >>>> checkpoint ? >>>> >>>> currently this assigned to -1. >>>> >>>> -AVM >>>> >>>> On 4/25/2017 6:58 PM, Canh Van Truong wrote: >>>> >>>>> Handling of checkpoint for stream open is in serveral places. >>>> Handling of checkpoint >>>> >>>>> is called in proc_stream_open_msg forgot to add checkpoint of >>>> destination name. >>>> >>>>> Refactor so that handling of checkpoint data for stream open is done >>>> in one place >>>> >>>>> and destination name was already added in checkpoint data at this place >>>>> --- >>>>> src/log/logd/lgs_evt.cc | 69 >>>> ++-------------------------------------------- >>>> >>>>> src/log/logd/lgs_imm.cc | 40 ++------------------------- >>>>> src/log/logd/lgs_mbcsv.cc | 15 +++++----- >>>>> src/log/logd/lgs_mbcsv.h | 5 ++-- >>>>> src/log/logd/lgs_stream.cc | 41 ++++++++++++++++++++++++++- >>>>> src/log/logd/lgs_stream.h | 2 ++ >>>>> 6 files changed, 57 insertions(+), 115 deletions(-) >>>>> diff --git a/src/log/logd/lgs_evt.cc b/src/log/logd/lgs_evt.cc >>>>> index 6972efe55..beb46e7a7 100644 >>>>> --- a/src/log/logd/lgs_evt.cc >>>>> +++ b/src/log/logd/lgs_evt.cc >>>>> @@ -821,72 +821,6 @@ snd_rsp: >>>>> } >>>>> /** >>>>> - * Stream open checkpointing >>>>> - * @param cb >>>>> - * @param logStream >>>>> - * @param open_sync_param >>>>> - * @return >>>>> - */ >>>>> -static uint32_t lgs_ckpt_stream_open(lgs_cb_t *cb, log_stream_t >>>> *logStream, >>>> >>>>> - lgsv_stream_open_req_t >>>> *open_sync_param) { >>>> >>>>> - uint32_t async_rc = NCSCC_RC_SUCCESS; >>>>> - lgsv_ckpt_msg_v1_t ckpt_v1; >>>>> - lgsv_ckpt_msg_v2_t ckpt_v2; >>>>> - void *ckpt_ptr; >>>>> - lgsv_ckpt_header_t *header_ptr; >>>>> - lgs_ckpt_stream_open_t *ckpt_rec_open_ptr; >>>>> - >>>>> - TRACE_ENTER(); >>>>> - >>>>> - if (lgs_is_peer_v2()) { >>>>> - memset(&ckpt_v2, 0, sizeof(ckpt_v2)); >>>>> - header_ptr = &ckpt_v2.header; >>>>> - ckpt_rec_open_ptr = &ckpt_v2.ckpt_rec.stream_open; >>>>> - ckpt_ptr = &ckpt_v2; >>>>> - } else { >>>>> - memset(&ckpt_v1, 0, sizeof(ckpt_v1)); >>>>> - header_ptr = &ckpt_v1.header; >>>>> - ckpt_rec_open_ptr = &ckpt_v1.ckpt_rec.stream_open; >>>>> - ckpt_ptr = &ckpt_v1; >>>>> - } >>>>> - >>>>> - if (cb->ha_state == SA_AMF_HA_ACTIVE) { >>>>> - header_ptr->ckpt_rec_type = LGS_CKPT_OPEN_STREAM; >>>>> - header_ptr->num_ckpt_records = 1; >>>>> - header_ptr->data_len = 1; >>>>> - ckpt_rec_open_ptr->clientId = open_sync_param->client_id; >>>>> - ckpt_rec_open_ptr->streamId = logStream->streamId; >>>>> - >>>>> - ckpt_rec_open_ptr->logFile = >>>>> - const_cast<char *>(logStream->fileName.c_str()); >>>>> - ckpt_rec_open_ptr->logPath = >>>>> - const_cast<char *>(logStream->pathName.c_str()); >>>>> - ckpt_rec_open_ptr->logFileCurrent = >>>>> - const_cast<char *>(logStream->logFileCurrent.c_str()); >>>>> - ckpt_rec_open_ptr->fileFmt = logStream->logFileFormat; >>>>> - ckpt_rec_open_ptr->logStreamName = >>>>> - const_cast<char *>(logStream->name.c_str()); >>>>> - >>>>> - ckpt_rec_open_ptr->maxFileSize = logStream->maxLogFileSize; >>>>> - ckpt_rec_open_ptr->maxLogRecordSize = logStream- >>> fixedLogRecordSize; >>>>> - ckpt_rec_open_ptr->logFileFullAction = logStream->logFullAction; >>>>> - ckpt_rec_open_ptr->maxFilesRotated = logStream->maxFilesRotated; >>>>> - ckpt_rec_open_ptr->creationTimeStamp = logStream- >>> creationTimeStamp; >>>>> - ckpt_rec_open_ptr->numOpeners = logStream->numOpeners; >>>>> - >>>>> - ckpt_rec_open_ptr->streamType = logStream->streamType; >>>>> - ckpt_rec_open_ptr->logRecordId = logStream->logRecordId; >>>>> - >>>>> - async_rc = lgs_ckpt_send_async(cb, ckpt_ptr, >> NCS_MBCSV_ACT_ADD); >>>>> - if (async_rc == NCSCC_RC_SUCCESS) { >>>>> - TRACE_4("REG_REC ASYNC UPDATE SEND SUCCESS..."); >>>>> - } >>>>> - } >>>>> - TRACE_LEAVE2("async_rc = %d", async_rc); >>>>> - return async_rc; >>>>> -} >>>>> - >>>>> -/** >>>>> * Create a new application stream >>>>> * >>>>> * @param open_sync_param[in] Parameters used to create the >> stream >>>>> @@ -1226,8 +1160,9 @@ snd_rsp: >>>>> rc = lgs_mds_msg_send(cb, &msg, &evt->fr_dest, &evt->mds_ctxt, >>>>> MDS_SEND_PRIORITY_HIGH); >>>>> + // Checkpoint the opened stream >>>>> if (ais_rv == SA_AIS_OK) { >>>>> - (void)lgs_ckpt_stream_open(cb, logStream, open_sync_param); >>>>> + (void)lgs_ckpt_stream_open(logStream, open_sync_param->client_id); >>>>> } >>>>> // These memories are allocated in MDS log open decode callback. >>>>> diff --git a/src/log/logd/lgs_imm.cc b/src/log/logd/lgs_imm.cc >>>>> index caf0cc92a..2eb3b7544 100644 >>>>> --- a/src/log/logd/lgs_imm.cc >>>>> +++ b/src/log/logd/lgs_imm.cc >>>>> @@ -318,43 +318,6 @@ static uint32_t >> ckpt_stream_config(log_stream_t >>>> *stream) { >>>> >>>>> } >>>>> /** >>>>> - * Pack and send an open stream checkpoint using mbcsv >>>>> - * @param stream >>>>> - * @return uint32 >>>>> - */ >>>>> -static uint32_t ckpt_stream_open(log_stream_t *stream) { >>>>> - uint32_t rc; >>>>> - lgsv_ckpt_msg_v1_t ckpt_v1; >>>>> - lgsv_ckpt_msg_v2_t ckpt_v2; >>>>> - void *ckpt_ptr; >>>>> - lgs_ckpt_stream_open_t *stream_open_ptr; >>>>> - lgsv_ckpt_header_t *header_ptr; >>>>> - >>>>> - TRACE_ENTER(); >>>>> - >>>>> - if (lgs_is_peer_v2()) { >>>>> - memset(&ckpt_v2, 0, sizeof(ckpt_v2)); >>>>> - header_ptr = &ckpt_v2.header; >>>>> - stream_open_ptr = &ckpt_v2.ckpt_rec.stream_open; >>>>> - ckpt_ptr = &ckpt_v2; >>>>> - } else { >>>>> - memset(&ckpt_v1, 0, sizeof(ckpt_v1)); >>>>> - header_ptr = &ckpt_v1.header; >>>>> - stream_open_ptr = &ckpt_v1.ckpt_rec.stream_open; >>>>> - ckpt_ptr = &ckpt_v1; >>>>> - } >>>>> - header_ptr->ckpt_rec_type = LGS_CKPT_OPEN_STREAM; >>>>> - header_ptr->num_ckpt_records = 1; >>>>> - header_ptr->data_len = 1; >>>>> - >>>>> - lgs_ckpt_stream_open_set(stream, stream_open_ptr); >>>>> - rc = lgs_ckpt_send_async(lgs_cb, ckpt_ptr, NCS_MBCSV_ACT_ADD); >>>>> - >>>>> - TRACE_LEAVE(); >>>>> - return rc; >>>>> -} >>>>> - >>>>> -/** >>>>> * Pack and send a close stream checkpoint using mbcsv >>>>> * @param stream >>>>> * @param recType >>>>> @@ -2409,7 +2372,8 @@ static void stream_ccb_apply_create(const >>>> CcbUtilOperationData_t *opdata) { >>>> >>>>> if ((rc = stream_create_and_configure1(opdata, &stream)) == >>>> SA_AIS_OK) { >>>> >>>>> log_stream_open_fileinit(stream); >>>>> - ckpt_stream_open(stream); >>>>> + // Checkpoint the opened stream with invalid clientId (-1) >>>>> + lgs_ckpt_stream_open(stream, -1); >>>>> } else { >>>>> LOG_IN("Stream create and configure failed %d", rc); >>>>> } >>>>> diff --git a/src/log/logd/lgs_mbcsv.cc b/src/log/logd/lgs_mbcsv.cc >>>>> index 59232e138..b94c8cad2 100644 >>>>> --- a/src/log/logd/lgs_mbcsv.cc >>>>> +++ b/src/log/logd/lgs_mbcsv.cc >>>>> @@ -670,16 +670,18 @@ static uint32_t >>>> ckpt_enc_cold_sync_data(lgs_cb_t *lgs_cb, >>>> >>>>> } /*End ckpt_enc_cold_sync_data() */ >>>>> /** >>>>> - * Set parameters for open stream >>>>> + * Set parameters for check-pointing the opened stream >>>>> * >>>>> * @param logStream >>>>> * @param stream_open >>>>> + * @param client_id >>>>> * @return >>>>> */ >>>>> -uint32_t lgs_ckpt_stream_open_set(log_stream_t *logStream, >>>>> - lgs_ckpt_stream_open_t *stream_open) { >>>>> +void lgs_ckpt_stream_open_set(log_stream_t *logStream, >>>>> + lgs_ckpt_stream_open_t *stream_open, >>>>> + const uint32_t &client_id) { >>>>> memset(stream_open, 0, sizeof(lgs_ckpt_stream_open_t)); >>>>> - stream_open->clientId = -1; /* not used in this message */ >>>>> + stream_open->clientId = client_id; >>>>> stream_open->streamId = logStream->streamId; >>>>> stream_open->logFile = const_cast<char >>>> *>(logStream->fileName.c_str()); >>>> >>>>> stream_open->logPath = const_cast<char >>>> *>(logStream->pathName.c_str()); >>>> >>>>> @@ -697,8 +699,6 @@ uint32_t >> lgs_ckpt_stream_open_set(log_stream_t >>>> *logStream, >>>> >>>>> stream_open->numOpeners = logStream->numOpeners; >>>>> stream_open->streamType = logStream->streamType; >>>>> stream_open->logRecordId = logStream->logRecordId; >>>>> - >>>>> - return NCSCC_RC_SUCCESS; >>>>> } >>>>> /** >>>>> @@ -737,7 +737,8 @@ static uint32_t edu_enc_streams(lgs_cb_t *cb, >>>> NCS_UBAID *uba) { >>>> >>>>> SaBoolT endloop = SA_FALSE, jstart = SA_TRUE; >>>>> while ((log_stream_rec = iterate_all_streams(endloop, jstart)) >>>> && !endloop) { >>>> >>>>> jstart = SA_FALSE; >>>>> - lgs_ckpt_stream_open_set(log_stream_rec, ckpt_stream_rec); >>>>> + // Encode the stream with invalid clientId for cold sync >>>>> + lgs_ckpt_stream_open_set(log_stream_rec, ckpt_stream_rec, -1); >>>>> rc = m_NCS_EDU_EXEC(&cb->edu_hdl, edp_ed_open_stream_rec, >> uba, >>>>> EDP_OP_TYPE_ENC, ckpt_stream_rec, &ederror); >>>>> diff --git a/src/log/logd/lgs_mbcsv.h b/src/log/logd/lgs_mbcsv.h >>>>> index b63b0cc2d..75a96deb6 100644 >>>>> --- a/src/log/logd/lgs_mbcsv.h >>>>> +++ b/src/log/logd/lgs_mbcsv.h >>>>> @@ -113,8 +113,9 @@ bool lgs_is_peer_v6(); >>>>> bool lgs_is_split_file_system(); >>>>> uint32_t lgs_mbcsv_dispatch(NCS_MBCSV_HDL mbcsv_hdl); >>>>> void lgs_free_edu_mem(char *ptr); >>>>> -uint32_t lgs_ckpt_stream_open_set(log_stream_t *logStream, >>>>> - lgs_ckpt_stream_open_t *stream_open); >>>>> +void lgs_ckpt_stream_open_set(log_stream_t *logStream, >>>>> + lgs_ckpt_stream_open_t *stream_open, >>>>> + const uint32_t &client_id); >>>>> uint32_t edp_ed_header_rec(EDU_HDL *edu_hdl, EDU_TKN *edu_tkn, >>>> NCSCONTEXT ptr, >>>> >>>>> uint32_t *ptr_data_len, EDU_BUF_ENV *buf_env, >>>>> EDP_OP_TYPE op, EDU_ERR *o_err); >>>>> diff --git a/src/log/logd/lgs_stream.cc b/src/log/logd/lgs_stream.cc >>>>> index 69da6c10e..26014c936 100644 >>>>> --- a/src/log/logd/lgs_stream.cc >>>>> +++ b/src/log/logd/lgs_stream.cc >>>>> @@ -31,9 +31,11 @@ >>>>> #include <algorithm> >>>>> #include "log/logd/lgs.h" >>>>> -#include "lgs_config.h" >>>>> +#include "log/logd/lgs_config.h" >>>>> #include "log/logd/lgs_file.h" >>>>> #include "log/logd/lgs_filehdl.h" >>>>> +#include "log/logd/lgs_mbcsv_v1.h" >>>>> +#include "log/logd/lgs_mbcsv_v2.h" >>>>> #include "base/osaf_time.h" >>>>> #include "osaf/immutil/immutil.h" >>>>> @@ -1597,3 +1599,40 @@ void >> log_stream_form_dest_names(log_stream_t >>>> *stream) { >>>> >>>>> } >>>>> stream->stb_dest_names = output; >>>>> } >>>>> + >>>>> +/** >>>>> +* Check-pointing the opened stream >>>>> +* @param stream >>>>> +* @param client_id >>>>> +* @return >>>>> +*/ >>>>> +void lgs_ckpt_stream_open(log_stream_t *stream, const uint32_t >>>> &client_id) { >>>> >>>>> + uint32_t rc; >>>>> + lgsv_ckpt_msg_v1_t ckpt_v1; >>>>> + lgsv_ckpt_msg_v2_t ckpt_v2; >>>>> + void *ckpt_ptr; >>>>> + lgs_ckpt_stream_open_t *stream_open_ptr; >>>>> + lgsv_ckpt_header_t *header_ptr; >>>>> + >>>>> + TRACE_ENTER(); >>>>> + >>>>> + if (lgs_is_peer_v2()) { >>>>> + memset(&ckpt_v2, 0, sizeof(ckpt_v2)); >>>>> + header_ptr = &ckpt_v2.header; >>>>> + stream_open_ptr = &ckpt_v2.ckpt_rec.stream_open; >>>>> + ckpt_ptr = &ckpt_v2; >>>>> + } else { >>>>> + memset(&ckpt_v1, 0, sizeof(ckpt_v1)); >>>>> + header_ptr = &ckpt_v1.header; >>>>> + stream_open_ptr = &ckpt_v1.ckpt_rec.stream_open; >>>>> + ckpt_ptr = &ckpt_v1; >>>>> + } >>>>> + header_ptr->ckpt_rec_type = LGS_CKPT_OPEN_STREAM; >>>>> + header_ptr->num_ckpt_records = 1; >>>>> + header_ptr->data_len = 1; >>>>> + >>>>> + lgs_ckpt_stream_open_set(stream, stream_open_ptr, client_id); >>>>> + rc = lgs_ckpt_send_async(lgs_cb, ckpt_ptr, NCS_MBCSV_ACT_ADD); >>>>> + >>>>> + TRACE_LEAVE2("Check-pointing the opened stream: rc=%d", rc); >>>>> +} >>>>> diff --git a/src/log/logd/lgs_stream.h b/src/log/logd/lgs_stream.h >>>>> index 1ed4b1571..0ef5b7a11 100644 >>>>> --- a/src/log/logd/lgs_stream.h >>>>> +++ b/src/log/logd/lgs_stream.h >>>>> @@ -143,4 +143,6 @@ void >> log_stream_delete_dest_name(log_stream_t >>>> *stream, >>>> >>>>> const std::vector<std::string> &names); >>>>> void log_stream_form_dest_names(log_stream_t *stream); >>>>> +void lgs_ckpt_stream_open(log_stream_t *stream, const uint32_t >>>> &client_id); >>>> >>>>> + >>>>> #endif // LOG_LOGD_LGS_STREAM_H_ > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
