Re: [asterisk-dev] [Code Review] 4612: Command cdr show pgsql status dont work
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4612/ --- (Updated April 16, 2015, 8:32 a.m.) Status -- This change has been discarded. Review request for Asterisk Developers. Bugs: https://issues.asterisk.org/jira/browse/ASTERISK-24959 https://issues.asterisk.org/jira/browse/https://issues.asterisk.org/jira/browse/ASTERISK-24959 Repository: Asterisk Description --- The command on cli cdr show pgsql status ever show usage option Usage: cdr show pgsql status Shows current connection status for cdr_pgsql Diffs - /trunk/cdr/cdr_pgsql.c 434725 Diff: https://reviewboard.asterisk.org/r/4612/diff/ Testing --- Thanks, Rodrigo Ramirez Norambuena -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4490: astdb: Allow clustering of the Asterisk Database between multiple Asterisk servers
On Wed, Apr 15, 2015 at 2:11 PM, Mark Michelson mmichel...@digium.com wrote: On 04/14/2015 12:11 PM, Matthew Jordan wrote: The question is: is this change worth having, or should it be scrapped in favour of some alternate approach that makes use of other technology? My feelings won't be hurt if the answer is ditch it and do something else - this was a fun piece of code to right on some plane flights. On the other hand, I don't have any real interest in writing an alternative approach, so if the expectation is that an AstDB wrapper around RabbitMQ or Redis will magically appear if I hit the delete key, that expectation is likely to be wrong. Personally, I like the idea of either 2) Allowing the AstDB to use a remote key-value store, thus allowing multiple Asterisk boxes to share the same store. I think that it would be best to develop an interface to third-party key-value stores and let them handle the hard bits. Personally, I like CoreOS' etcd, but there are others that could be useful like ZooKeeper, Consul or Redis. -- Jeff Ollie -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4490: astdb: Allow clustering of the Asterisk Database between multiple Asterisk servers
This is definitely the approach that I would prefer as well, as it makes integration with external software much easier. - Brad On Thu, Apr 16, 2015 at 12:37 PM, Jeffrey Ollie j...@ocjtech.us wrote: On Wed, Apr 15, 2015 at 2:11 PM, Mark Michelson mmichel...@digium.com wrote: On 04/14/2015 12:11 PM, Matthew Jordan wrote: The question is: is this change worth having, or should it be scrapped in favour of some alternate approach that makes use of other technology? My feelings won't be hurt if the answer is ditch it and do something else - this was a fun piece of code to right on some plane flights. On the other hand, I don't have any real interest in writing an alternative approach, so if the expectation is that an AstDB wrapper around RabbitMQ or Redis will magically appear if I hit the delete key, that expectation is likely to be wrong. Personally, I like the idea of either 2) Allowing the AstDB to use a remote key-value store, thus allowing multiple Asterisk boxes to share the same store. I think that it would be best to develop an interface to third-party key-value stores and let them handle the hard bits. Personally, I like CoreOS' etcd, but there are others that could be useful like ZooKeeper, Consul or Redis. -- Jeff Ollie -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] Git Migration update
On Tue, Apr 14, 2015 at 9:24 AM, Russell Bryant russ...@russellbryant.net wrote: On Mon, Apr 13, 2015 at 6:52 PM, Matthew Jordan mjor...@digium.com wrote: For *right now*, we are going to try cherry-picking the changes to the affected branches when the change is first up for review. This is clearly a pretty big change in process, as the act of merging into other branches was (a) always done by those with commit access and (b) never reviewed. There's at least two good reasons to give this a try: (1) There isn't anyone with commit access. Anyone can post a review up to Gerrit. The plus side is that there are far fewer barriers to getting a patch into Asterisk. The downside is that there isn't a select group of people who have been trusted to do the merges. The only way to ensure that patches actually do get merged into all the branches is to require people to put the patches up with the initial review. (2) Gerrit really, really wants to review things. That's a good thing: we've had plenty of bad merges take out branches in the past - either from compilation issues, subtle bugs that creep in due to API compatibility problems, etc. We've had even more that get merged upstream and fail to take advantage of APIs that exist in later versions of Asterisk. Reviews will help to catch that. This is a trial. We'll re-evaluate how things are working at the end of the week. So, tentatively, I think the model of cherry-picking changes to all affected branches (including master) is working out okay. This does mean the volume of code reviews has gone up, but we appear to be keeping reasonable pace with the changes. One of the benefits of Gerrit is that while the volume of code reviews is greater, it is: (1) FAR easier to get changes merged in, particularly when the entire change is now reviewed for all branches. (2) Much more collaborative, as anyone can address findings and/or fix up commits and update reviews. As such, I've gone ahead and updated the various wiki pages (with some help from Corey and George) to note the Git related policies. The major pages are: * Patch Contribution Process (https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process) A very high level page that details for new(er) developers how to submit a patch to the project. It's also a good overview for everyone, and links to many of the pages below. * Git Usage (https://wiki.asterisk.org/wiki/display/AST/Git+Usage) This brief page lists out the various Git repositories (mirrors and the like), and specific recommendations for how the -2+2 reviews should be handled. * Gerrit Usage (https://wiki.asterisk.org/wiki/display/AST/Gerrit+Usage) This rather detailed page contains instructions for using Gerrit and git review. * Commit Messages (https://wiki.asterisk.org/wiki/display/AST/Commit+Messages) Details proper formatting of commit messages. Since the commit messages are also reviewed, this is important for everyone. * Software Configuration Management Policies (https://wiki.asterisk.org/wiki/display/AST/Software+Configuration+Management+Policies) Details the types of branches in Asterisk (Standard vs LTS), new feature/bug fix policies, as well as the currently supported branches. Most of this should be familiar to long-time developers of Asterisk, and not much has changed here. I'm sure there is information that is still missing or unclear. If you have any questions about the content on the pages above, please feel free to ask here on the mailing list or in #asterisk-dev, and I (or others) will work to get them cleared up. Thanks! Matt -- Matthew Jordan Digium, Inc. | Director of Technology 445 Jan Davis Drive NW - Huntsville, AL 35806 - USA Check us out at: http://digium.com http://asterisk.org -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
[asterisk-dev] A Week with GIT/Gerrit
The Emails: Overall I think they're too verbose. Change in asterisk[master]: bridge.c: NULL app causes crash during attended transfer might be better as bridge.c: NULL app causes crash during attended transfer (asterisk[master]) It's not a lot shorter but it has the most valuable info at the front. Also, the mail generated by ReplacePatchSet which gets sent on a rebase, doesn't tell you why you're getting the message but it does have the entire original commit message which doesn't really help. I'd remove the commit message and add the text of the last change. I.E. Foo Bar: Patch Set 8: Patch Set 7 was rebased. That'll at least tell reviewers not to worry about it. Removing (Code Review) from the sender would be good as well. Cherry-picking: I think there are some issues we still need to figure out about the process and the system. Process first... While having all the reviews up at the same time is a good thing, I think we need to always start with the lowest branch the patch is expected to apply to and merge up towards master. I think the wiki says merge down for features and merge up for bugs. It should be up always. You don't want to start a feature in master only to discover that it can't go into 13 because some other feature is only in master. Same for review process. Start in the lowest branch and work up. This will hopefully minimize where different people are starting from different directions. It's hard to follow feedback and ensure you've got all the comments covered like that. Something I've noticed though is that dependencies get messed up when cherry-picking a series of dependent patches. Look at my patches 46, 47, 48 for master. If I cherry-pick 48 to my local repo, I get all 3 which is good because I can't test any without all 3. BUT now look at 43, 44, 45 for 13. When I check out 45, I get only 45 which is an issue. Maybe this is just an artifact of this being the first time with this scenario but it's something to look out for. Other things: There's a rebase button on reviews which need it but if you click it, you lose you're votes. :( We probably need a policy around rebasing reviews. I've noticed a couple of times where doing a 'git review' regenerated the Change ID and I can't figure out why. I've resorted to running 'git review -n' to see the commands and just running the 'git push' part. I'm still investigating. If you move between major releases you'll find various orphaned files and directories that are part of one release but not antother. Use 'git clean -fd' to clean them up. BEWARE: this will remove all files/directories that aren't part of the current branch or in the .gitignore files. If you have local scripts or other stuff you want to keep between checkouts, add them to .git/info/exclude. If you're a Google Apps/Gmail user use the filter list:gerrit.asterisk.org to filter all messages regardless of repo. If you're switching between branches with the same major version a lot, install ccache! You're compiles will go a lot faster. When doing a 'git review' to update a review, you don't have to specify the topic branch again but you DO have to specify the target branch if it's not master. We could still use the links to the specific git/gerrit wiki pages on the gerrit menu. I think that's it for now. -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] A Week with GIT/Gerrit
On Thu, Apr 16, 2015 at 6:00 PM, George Joseph george.jos...@fairview5.com wrote: Something I've noticed though is that dependencies get messed up when cherry-picking a series of dependent patches. Look at my patches 46, 47, 48 for master. If I cherry-pick 48 to my local repo, I get all 3 which is good because I can't test any without all 3. BUT now look at 43, 44, 45 for 13. When I check out 45, I get only 45 which is an issue. Maybe this is just an artifact of this being the first time with this scenario but it's something to look out for. This is likely because for 13, the changes were cherry picked individually as changes against HEAD of 13. They need to be set up as 3 commits in a series on top of 13 and submitted that way. Backporting in that case will need to be done locally to preserve those dependencies instead of clicking the cherry-pick buttons. -- Russell Bryant -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] A Week with GIT/Gerrit
On Thu, Apr 16, 2015 at 4:11 PM, Russell Bryant russ...@russellbryant.net wrote: On Thu, Apr 16, 2015 at 6:00 PM, George Joseph george.jos...@fairview5.com wrote: Something I've noticed though is that dependencies get messed up when cherry-picking a series of dependent patches. Look at my patches 46, 47, 48 for master. If I cherry-pick 48 to my local repo, I get all 3 which is good because I can't test any without all 3. BUT now look at 43, 44, 45 for 13. When I check out 45, I get only 45 which is an issue. Maybe this is just an artifact of this being the first time with this scenario but it's something to look out for. This is likely because for 13, the changes were cherry picked individually as changes against HEAD of 13. They need to be set up as 3 commits in a series on top of 13 and submitted that way. Backporting in that case will need to be done locally to preserve those dependencies instead of clicking the cherry-pick buttons. Ah, that makes sense now. We should probably note that on the wiki. -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4391: Add blank line between headers and output for Command action response
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4391/ --- (Updated April 17, 2015, 3:45 p.m.) Status -- This change has been discarded. Review request for Asterisk Developers. Bugs: ASTERISK-24730 https://issues.asterisk.org/jira/browse/ASTERISK-24730 Repository: Asterisk Description --- This patch adds a blank line between the headers and the output in the Command action response. The reason for this change is to make it easier to determine where the headers end and the output from the command starts. Currently, to parse a response to a Command action, one has to: 1. Read one line, if line is Response: Error, parse the remaining as a standard AMI response and stop. 2. Read one more line - or two if you used an ActionID - those lines are the headers. 3. Then read everything up to --END COMMAND--\r\n\r\n. That could be changed to: 1. Read standard AMI response. 2. If Response: Follows, then read everything up to --END COMMAND--\r\n\r\n. The AMI version has been increased to 2.8.0 as this is a protocol change and so that clients detect the new behavior. Adding a blank line should not cause older parsers to fail as they have to read everything up to --END COMMAND--\r\n\r\n anyway. Adding a blank line will also not cause the AMI to HTML/XML encoder to fail to separate the headers from the output as it checks for the presence of a : character, which a blank line does not contain. Diffs - /trunk/main/manager.c 434448 /trunk/main/cli.c 434448 /trunk/include/asterisk/manager.h 434448 /trunk/UPGRADE.txt 434448 Diff: https://reviewboard.asterisk.org/r/4391/diff/ Testing --- Connected to manager, issued 'core show uptime' command and verified that there was a blank line between the headers and output. Thanks, gareth -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
