Camera support

Which device would you like us to support?
Outdoor Camera (ADC-V723): https://www.telus.com/en/smarthome-security/security-system-products/security-outdoor-camera
Where does this device appear in the Alarm.com mobile app?
Video
What types of actions does this device support?
E.g.: Garage doors support opening and closing. Alarm systems support arming (home, away, and night modes) and disarming.

View video from camera
Take snapshot
Request recording

Which provider do you use?
Telus (formerly ADT Canada)
Include pyalarmdotcomajax Server Output
No information regarding the camera after running adc -u USERNAME -p PASSWORD.
Include Action Endpoints
Base URL: https://www.alarm.com/web/api/video
Take snapshot suffix: snapshots/<camera-id>
Request recording suffix: requestRecording?id=<camera-id>
Some other (probably useful) endpoints
WebRTC sources suffix (GET): liveVideoHighestResSources/<camera-id>
WebRTC initial stream log send (POST): streamLogs/webrtcStreamLogs/sendInitialWebrtcStreamLog
Attributes of all cameras (GET): cameras

@AritroSaha10 There is a pretty popular WebRTC component for Home Assistant: https://github.com/AlexxIT/WebRTC. Other integrations use this component to display their streams. This is probably a good candidate for integration with ADC cameras. I don't have an ADC camera, though, so I'd be looking to you to drive this effort.
There are two obvious approaches here:

Access the stream made available via the Alarm.com API (looks like you already identified the endpoints). This is probably the more stable approach.
Access the camera directly using the approach outlined in https://github.com/uvjustin/alarmdotcom/issues/50#issuecomment-1019305600. This is probably more resilient as this approach lets you access the cameras over your LAN.

I'll try to use the WebRTC component code, thanks. Though, I believe the 2nd method might not be possible since some camera's don't use MJPEG (ex. the ADC-V723, the one I have).
However, it might be possible to use RTSP with some work. I did some digging and found out that there is a feature on the Qolsys IQ Panel 2 where you can stream the camera to it directly (?). It says that both of them must be on the same network for the feature to work, so it implies something direct between the two devices?
Right now, I'm trying to figure out how to get the stream working through Alarm.com, but I'm not completely where to start as I'm unfamiliar with WebRTC, and I don't know how I can use the TURN/STUN servers and Janus webhooks provided by the endpoints to view the stream anywhere else than the alarm.com website. In addition, it seems like the proxy url is encrypted? Not sure where to start with that either.

I would like support for the motiondetector with camera

Posting data provided by @karthikmannem1991:
ADC-V723x (#217):
https://www.alarmsystemstore.com/products/alarm-com-adc-v723-outdoor-1080p-wi-fi-camera
{
    "data": [
        {
            "id": "102347524-2049",
            "type": "video/devices/camera",
            "attributes": {
                "supportsSnapShot": true,
                "canTakeSnapshot": true,
                "supportsRecording": true,
                "canRequestRecording": true,
                "canUploadClips": true,
                "maxSvrClipDownloadDurationSec": 3600,
                "username": "123",
                "hasSvrSchedule": false,
                "onvifParentId": null,
                "excludedFromVisualVerification": false,
                "excludedFromEscalatedEvents": false,
                "liveViewHdToggleEnabled": true,
                "supportsEndToEndWebRTCStreaming": true,
                "supports4MPStreaming": false,
                "supportsSiren": false,
                "canSirenBeActivatedByLogin": false,
                "isSirenEnabledInSettings": false,
                "isSirenPlaying": false,
                "osdOverlayEnabled": false,
                "canPanTilt": false,
                "maxPanTiltLevel": 0,
                "canZoom": false,
                "maxZoomLevel": 0,
                "minZoomLevelForPanTilt": 0,
                "panTiltPresets": [],
                "preferredPanTiltPreset": null,
                "isUnreachable": false,
                "lastSuccessfulSupervision": "2023-02-24T13:13:46.957Z",
                "canStreamToPanel": true,
                "canChangeStreamToPanel": true,
                "canStreamToPanelBeEnabled": true,
                "isStreamToPanelEnabled": true,
                "doorbellChimeType": -1,
                "supportedDoorbellWedgeOptions": null,
                "doorbellWedgeType": 0,
                "supportsWedgeYesNo": false,
                "supportsWedgeGIF": false,
                "supportsVmdSchedules": true,
                "deviceImagePath": "/web/webimages/camera_images/V723_64px.png?v=2",
                "deviceModel": "ADC-V723X",
                "macAddress": "xxxxxxxxxx",
                "firmwareVersion": "<IP_ADDRESS>0",
                "publicIp": "xxxxxxxxxx",
                "privateIp": "xxxxxxxx",
                "port": "40926",
                "httpsPort": "41926",
                "shouldUseEntireImageForSnapshot": false,
                "hasDdnsed": true,
                "usageProtocolMapping": {
                    "18": 1,
                    "9": 1,
                    "10": 1,
                    "6": 1,
                    "20": 1
                },
                "isVirtualCamera": false,
                "isVarifocalCamera": false,
                "hasSdRecordingSchedule": false,
                "isBatteryCameraAndUsingGatewayForBackendAccess": false,
                "supportsLiveView": true,
                "forcedAspectRatio": 1.77777779,
                "canBeSaved": false,
                "canChangeDescription": false,
                "description": "myCamera",
                "deviceModelId": 0,
                "canConfirmStateChange": false,
                "canReceiveCommands": false,
                "remoteCommandsEnabled": false,
                "hasPermissionToChangeState": false,
                "deviceIcon": {
                    "icon": 27
                },
                "batteryLevelNull": null,
                "lowBattery": false,
                "criticalBattery": false
            },
            "relationships": {
                "videoEvents": {
                    "data": [],
                    "meta": {
                        "count": "0"
                    }
                },
                "password": {
                    "data": {
                        "id": "102347524-2049",
                        "type": "video/cameraPassword"
                    }
                },
                "videoSource": {
                    "data": {
                        "id": "102347524-2049",
                        "type": "video/videoSources/liveVideoSource"
                    }
                },
                "videoSourceHd": {
                    "data": {
                        "id": "102347524-2049",
                        "type": "video/videoSources/liveVideoHighestResSource"
                    }
                },
                "svrVideoSource": {
                    "data": {
                        "id": "102347524-2049",
                        "type": "video/videoSources/svrVideoSource"
                    }
                },
                "svrScrubberThumbnailEndpoint": {
                    "data": {
                        "id": "102347524-2049",
                        "type": "video/svrThumbnailEndpoint"
                    }
                },
                "cameraSiren": {
                    "data": null
                },
                "liveVideoSetting": {
                    "data": {
                        "id": "102347524-2049",
                        "type": "video/live-video-setting"
                    }
                },
                "snapshot": {
                    "data": {
                        "id": "102347524-2049",
                        "type": "video/snapshot"
                    }
                },
                "privacyZonesSetting": {
                    "data": {
                        "id": "102347524-2049",
                        "type": "video/privacy-zones-setting"
                    }
                },
                "system": {
                    "data": {
                        "id": "12385880",
                        "type": "systems/system"
                    }
                },
                "stateInfo": {
                    "data": null
                }
            }
        }
    ],
    "included": [],
    "meta": {
        "transformer_version": "1.1"
    }
}

ADC-VDB780B (#216)
https://doorbell.poweredbyalarm.com/vdb780b-install/
{
            "id": "102347524-2051",
            "type": "video/devices/camera",
            "attributes": {
                "supportsSnapShot": true,
                "canTakeSnapshot": true,
                "supportsRecording": true,
                "canRequestRecording": true,
                "canUploadClips": true,
                "maxSvrClipDownloadDurationSec": 3600,
                "username": "123",
                "hasSvrSchedule": false,
                "onvifParentId": null,
                "excludedFromVisualVerification": false,
                "excludedFromEscalatedEvents": false,
                "liveViewHdToggleEnabled": true,
                "supportsEndToEndWebRTCStreaming": false,
                "supports4MPStreaming": false,
                "supportsSiren": false,
                "canSirenBeActivatedByLogin": false,
                "isSirenEnabledInSettings": false,
                "isSirenPlaying": false,
                "osdOverlayEnabled": false,
                "canPanTilt": false,
                "maxPanTiltLevel": 0,
                "canZoom": false,
                "maxZoomLevel": 0,
                "minZoomLevelForPanTilt": 0,
                "panTiltPresets": [],
                "preferredPanTiltPreset": null,
                "isUnreachable": false,
                "lastSuccessfulSupervision": "2023-02-12T02:20:36.293Z",
                "canStreamToPanel": true,
                "canChangeStreamToPanel": true,
                "canStreamToPanelBeEnabled": true,
                "isStreamToPanelEnabled": true,
                "doorbellChimeType": 1,
                "supportedDoorbellWedgeOptions": null,
                "doorbellWedgeType": 0,
                "supportsWedgeYesNo": false,
                "supportsWedgeGIF": false,
                "supportsVmdSchedules": false,
                "deviceImagePath": "/web/webimages/camera_images/VDB780B_64px.png?v=2",
                "deviceModel": "ADC-VDB780B",
                "macAddress": "xxxxxxxxxxx",
                "firmwareVersion": "<IP_ADDRESS>",
                "publicIp": "xxxxxxxxx",
                "privateIp": "xxxxxxxxx",
                "port": "80",
                "httpsPort": "443",
                "shouldUseEntireImageForSnapshot": false,
                "hasDdnsed": false,
                "usageProtocolMapping": {
                    "18": 1,
                    "9": 1,
                    "10": 1,
                    "6": 1,
                    "20": 1
                },
                "isVirtualCamera": false,
                "isVarifocalCamera": false,
                "hasSdRecordingSchedule": false,
                "isBatteryCameraAndUsingGatewayForBackendAccess": true,
                "supportsLiveView": true,
                "forcedAspectRatio": null,
                "canBeSaved": false,
                "canChangeDescription": false,
                "description": "Doorbell",
                "deviceModelId": 0,
                "canConfirmStateChange": false,
                "canReceiveCommands": false,
                "remoteCommandsEnabled": false,
                "hasPermissionToChangeState": false,
                "deviceIcon": {
                    "icon": 27
                },
                "batteryLevelNull": null,
                "lowBattery": false,
                "criticalBattery": false
            },
            "relationships": {
                "videoEvents": {
                    "data": [],
                    "meta": {
                        "count": "0"
                    }
                },
                "password": {
                    "data": {
                        "id": "102347524-2051",
                        "type": "video/cameraPassword"
                    }
                },
                "videoSource": {
                    "data": {
                        "id": "102347524-2051",
                        "type": "video/videoSources/liveVideoSource"
                    }
                },
                "videoSourceHd": {
                    "data": {
                        "id": "102347524-2051",
                        "type": "video/videoSources/liveVideoHighestResSource"
                    }
                },
                "svrVideoSource": {
                    "data": {
                        "id": "102347524-2051",
                        "type": "video/videoSources/svrVideoSource"
                    }
                },
                "svrScrubberThumbnailEndpoint": {
                    "data": {
                        "id": "102347524-2051",
                        "type": "video/svrThumbnailEndpoint"
                    }
                },
                "cameraSiren": {
                    "data": null
                },
                "liveVideoSetting": {
                    "data": {
                        "id": "102347524-2051",
                        "type": "video/live-video-setting"
                    }
                },
                "snapshot": {
                    "data": {
                        "id": "102347524-2051",
                        "type": "video/snapshot"
                    }
                },
                "privacyZonesSetting": {
                    "data": null
                },
                "system": {
                    "data": {
                        "id": "12385880",
                        "type": "systems/system"
                    }
                },
                "stateInfo": {
                    "data": null
                }
            }
        },

This might be of interest to you

Notes: the web viewer makes a call to https://www.alarm.com/web/api/video/videoSources/liveVideoHighestResSources/<CAM-ID> and receives a response which includes proxyUrl under it's attributes. This proxy url can be used to view the camera live through ADC's servers.


Bug: Timestamps not working

The timestamp template tags {{_created_at}} and {{_updated_at}} are not working. On the rendered page, they are displayed as {{section_name::_created_at}} and {{section_name::_updated_at}}.
To Reproduce
Steps to reproduce the behavior:

Add {{_created_at}} or {{_updated_at}} to a {{#section}} block
Visit the rendered page

Expected behavior
{{_created_at}} should return the record’s date of creation. {{_updated_at}} should return the date of the record’s last edit.
Your Environment

OS: macOS Catalina 10.15.4
Browser: Safari 13.1
Node Version: 8.16.2
Vapid Version: 0.9.6

@JohJakob Is this before or after you add posts to the section in the dashboard?

I'm having the same issue.
Your Environment

OS: Windows 10
Browser: Chrome Version 83.0.4103.61
Node Version: 8.11.1
Vapid Version: 9.9.6

@srobbin It happens after I add posts to the section.

@JohJakob, @Voydfox Thank you. It looks like it was a known bug with Sequelize that had an easy workaround. Try upgrading to Vapid v0.10.0 and let me know if it's fixed.
Note: Vapid v0.10.0 now requires Node >= v10, since Node v8 was end-of-lifed in December.

@srobbin it works, thank you kindly! <3


Clean up example template

We got an issue from manual testing in https://github.com/fusetools/Fuse/issues/4325, and after some discussion figured out the issue was with the example template itself.
See this comment by @mortoray  in particular: https://github.com/fusetools/Fuse/issues/4325#issuecomment-337167447

File an issue on whever "examples" issues go. The example should be cleaned up, no border on the image, better layout on that page.

I've attempted a fix here https://github.com/fusetools/fuselibs-public/pull/606 , but it seems to me like the bug we experience in the video is less due to the borders around the image and more because its using the default stretch mode: since the height is set to 500x500, which is much taller than the image is.


Nuclei

Nuclei remains indefinitely without advancing with only 1 domain
output:
[INF] Your current nuclei-templates v9.3.1 are outdated. Latest is v9.3.4
[INF] Downloading latest release...
[INF] Successfully updated nuclei-templates (v9.3.4) to /opt/nuclei-templates. GoodLuck!
[ERR] Could not read nuclei-ignore file: open /root/.config/nuclei/.nuclei-ignore: no such file or directory
[INF] Using Nuclei Engine 2.8.3 (outdated)
[INF] Using Nuclei Templates 9.3.4 (latest)
[INF] Templates added in last update: 15
[INF] Templates loaded for scan: 2817
[INF] Targets loaded for scan: 4
[INF] Templates clustered: 245 (Reduced 880 HTTP Requests)
[INF] Using Interactsh Server: oast.fun
Thanks

Hey! That's weird, I don't seem to be able to reproduce this issue with the current build. Are you using Docker or a local build? Does it work when you try with the latest Docker build, or on a different target?


Bit-vector Updates: fix issues, add function mode, trigger selection, examples, etc

Fixed problems that cause unsoundness and failure to active triggers:
In the prelude, the (integer version of) bit-wise operator's argument type is changed from int to poly. Also, it additionally takes in the bit-width of input arguments. For example, uninterpreted bitwise and is declared as (declare-fun [uint_and] (Int Poly Poly) Int). To be specific, the normal sst_to_air translation of bitwise operators is done by the following steps: 1) box the arguments, 2) apply the uninterpreted bit-wise operator, 3) apply uClip/iClip according to the bit-width.

Since the last PR, let, quantifier binding, if and else is added in the bit-vector support. The supported functionalities for bit-vector can be found in exp_to_bv_expr in sst_to_air.rs.

Bit-vector function mode is added, and it only suports empty body function. It will interpret requires and ensures as bit-vector, prove in bit-vector reasoning, and port it as integer axioms.

Examples are added.
example/bitvector_garbage.rs  contains the example that describes a 32-bit integer as an array of 16 elements.
docs/bv_experiments/bitmaps.rs contains the bitmap example, which is about the vector of u64, to represent arbitrary long bit-vectors. This example currently hangs on the default setting. It will work when the following option value is set as 0. (set-option :smt.case_split 3)
example/bitvector_basic.rs contains basic properties for bit-wise operators, such as commutativity, associativity, etc.

Automatic trigger selection for bitwise operators. A trigger containing a bitwise operator will have the lowest precedence in the selection process.

Are you planning to merge by squashing? If not, can you please clean up the history? (There are a lot of commits and reverts of merge commits, which doesn't look right.)
If instead, this is not ready for review, please mark it as draft.
Thanks!

Some initial comments. Thank you!

Thanks a lot for the comments! I updated it in the latest commit


Allow for moving truncation into the implementing API

As usual, not claiming this is the right way to do it, but I think it is a way to do it. We can then update the github-checks-plugin to call getSummary(65536, " see [Jenkins](url)") when creating its Output. It does mean that consumers can't set the 'see more' text, but it means that the GHChecks plugin can confidently use markdown.
Of course one of the other options is to add something to the GitHub Checks plugin that can intelligently truncate markdown. But I've no idea what sort of libraries are already out there to help with that. That would also prevent the user from being able to control chunk size.

'see more' text, but it means that the GHChecks plugin can confidently use markdown.

not ideal as in the junit plugin we link to the test report

intelligently truncate markdown.

would be surprised if we could get something that would work without it being overly complicated

'see more' text, but it means that the GHChecks plugin can confidently use markdown.

not ideal as in the junit plugin we link to the test report

intelligently truncate markdown.

would be surprised if we could get something that would work without it being overly complicated

not ideal as in the junit plugin we link to the test report

That does mean the junit plugin assumes that the checks api implementation supports markdown. That seems likely to be the case, but who knows.
We could make things considerably more structured - have something like a ChecksTruncatedText class that accepts a url and a list of strings, and then is invoked by the implementation that turns that constructs the read more link into the appropriate markdown and then does the chunking based on its own knowledge of maximum lengths - but that would be even bigger changes to the API.

not ideal as in the junit plugin we link to the test report

That does mean the junit plugin assumes that the checks api implementation supports markdown. That seems likely to be the case, but who knows.
We could make things considerably more structured - have something like a ChecksTruncatedText class that accepts a url and a list of strings, and then is invoked by the implementation that turns that constructs the read more link into the appropriate markdown and then does the chunking based on its own knowledge of maximum lengths - but that would be even bigger changes to the API.

I don't think we should validate the message here because the overflow should be a problem that the implementations should take care of IMO. Since when they are implementing this API according to a specific platform, they should refer to the official documents (e.g. Github documents for checks API) to do the validations. Maybe in the status publisher, it is ok to check the max size because it is a consumer that tries to make every work, but adding too much validation here makes the API complicated IMO.

I don't think we should validate the message here because the overflow should be a problem that the implementations should take care of IMO. Since when they are implementing this API according to a specific platform, they should refer to the official documents (e.g. Github documents for checks API) to do the validations. Maybe in the status publisher, it is ok to check the max size because it is a consumer that tries to make every work, but adding too much validation here makes the API complicated IMO.

I think thats maybe why I'd like to see some kind of TruncatedStringBuilder exposed from the checks publisher so we end up with something like:
new ChecksOutput.Builder()
  .withSummary(publisher.getTruncatedStringBuilder()
    .withReadMoreUrl(url)
    .addText(...)
    .addText(...)
    .build())

Then its up to the implementers to provide an implmentation of TruncatedStringBuilder that sets the correct limits, and converts the 'read more' url into the appropriate formatting.

I think thats maybe why I'd like to see some kind of TruncatedStringBuilder exposed from the checks publisher so we end up with something like:
new ChecksOutput.Builder()
  .withSummary(publisher.getTruncatedStringBuilder()
    .withReadMoreUrl(url)
    .addText(...)
    .addText(...)
    .build())

Then its up to the implementers to provide an implmentation of TruncatedStringBuilder that sets the correct limits, and converts the 'read more' url into the appropriate formatting.

Would the read more URL different from the details URL?

Would the read more URL different from the details URL?

Would the read more URL different from the details URL?

Good point. I'm not sure it would be. But possibly difficult to get to.

Would the read more URL different from the details URL?

Good point. I'm not sure it would be. But possibly difficult to get to.

I don't think we should validate the message here because the overflow should be a problem that the implementations should take care of IMO. Since when they are implementing this API according to a specific platform, they should refer to the official documents (e.g. Github documents for checks API) to do the validations. Maybe in the status publisher, it is ok to check the max size because it is a consumer that tries to make every work, but adding too much validation here makes the API complicated IMO.

the problem with implementations doing it is you would get broken markdown and truncating at a sane point would be difficult

Would the read more URL different from the details URL?

I don't think so

I don't think we should validate the message here because the overflow should be a problem that the implementations should take care of IMO. Since when they are implementing this API according to a specific platform, they should refer to the official documents (e.g. Github documents for checks API) to do the validations. Maybe in the status publisher, it is ok to check the max size because it is a consumer that tries to make every work, but adding too much validation here makes the API complicated IMO.

the problem with implementations doing it is you would get broken markdown and truncating at a sane point would be difficult

Would the read more URL different from the details URL?

I don't think so

Actually the worry about read more being in markdown is pointless as consumers are already assuming markdown as the format for long text fields. I guess if other implementations don’t support markdown they can handle that themselves.

Actually the worry about read more being in markdown is pointless as consumers are already assuming markdown as the format for long text fields. I guess if other implementations don’t support markdown they can handle that themselves.

So, condensing the above I think we have:

Consumers should not have to worry about truncating long text fields per-platform; this should be handled by the implementer.
We should not worry about the format of the truncation text, as the consumer is already providing the body text in whatever format they like. Perhaps we should document in the api that the consumers are expected to be able to handle markdown; if their platform does not then they may wish to convert/simplify it as appropriate.
Consumers should be able to specify their own truncation text.

Does that sound reasonable? I will work out some code roughly to that effect.

So, condensing the above I think we have:

Consumers should not have to worry about truncating long text fields per-platform; this should be handled by the implementer.
We should not worry about the format of the truncation text, as the consumer is already providing the body text in whatever format they like. Perhaps we should document in the api that the consumers are expected to be able to handle markdown; if their platform does not then they may wish to convert/simplify it as appropriate.
Consumers should be able to specify their own truncation text.

Does that sound reasonable? I will work out some code roughly to that effect.

Yep. I'll pull over (and extend) the tests from https://github.com/jenkinsci/checks-api-plugin/pull/66/

Yep. I'll pull over (and extend) the tests from https://github.com/jenkinsci/checks-api-plugin/pull/66/

Ok this may now be hugely over-engineered, but I think it works. The tests hopefully give an idea of what it can do.

Ok this may now be hugely over-engineered, but I think it works. The tests hopefully give an idea of what it can do.

Ok this may now be hugely over-engineered, but I think it works. The tests hopefully give an idea of what it can do.

seems sane any reason it's in draft?

Ok this may now be hugely over-engineered, but I think it works. The tests hopefully give an idea of what it can do.

seems sane any reason it's in draft?

I’ll rebase #66 on top of it to validate it first, if a rebase is ok - otherwise I’ll spin a new branch to test it.
I’ll also get a PR open for github checks.

I’ll rebase #66 on top of it to validate it first, if a rebase is ok - otherwise I’ll spin a new branch to test it.
I’ll also get a PR open for github checks.

I'm glad I tried using it on #66- definitely have some changes to make.

I'm glad I tried using it on #66- definitely have some changes to make.


WIP - Heartbeat support in vttablet

I'm creating tests and testing in my environment. Submitting PR to see what travis has to say.
Any comments welcome.

This change is

I've done a bunch of testing in our QA environment, but am having trouble writing new tests for this. I'm not sure if it's my environment or what, but fakesqldb seems very finicky and I've spent a while toying with it. The biggest problem is that it seems to frequently (not always) fail with errors coming from the C bindings, such as pointer being freed was not allocated. Any tips?

I haven't seen any problems so far.
Instead of using the C library, you could use our new native Go MySQL library. The easiest way to test it is probably by changing the default of the "-engine" flag to "mysqlconn": https://github.com/youtube/vitess/blob/master/go/vt/dbconfigs/dbconfigs.go#L49

If possible, I'd like to get to the bottom of this. Maybe this will expose an underlying problem. If I don't resolve it by tonight, we can try going the mysqlconn route.
Or you can go ahead with the mysqlconn route, but leave me a way to recreate the failure.

Thanks guys, I have an idea of why this may be. When I wrote this, I wasn't sure how to create a normal MySQL connection (unpooled) so I dug into the pooling code and pulled out the function from the pool factory: dbconnpool.NewDBConnection. Looking further into this now, I see that for connections made with this function, any errors in ExecuteFetch cause the connection to be closed. The idea being that the pool will create a new one, I guess.
Between the tests I'm writing throwing controlled errors, and the goroutine involved running as well, it's causing race conditions and the connections to be closed when it's not expected.
Any suggestions on how to create a dedicated connection for the heartbeat goroutine? Should I just create a pool of size 1? Should I use MysqlDaemon instead?

Yes. You should use a pool. You can use https://github.com/youtube/vitess/blob/master/go/vt/vttablet/tabletserver/twopc.go as an example. You can also copy the initialization patterns for creating table(s) (maybe you already did). Still haven't looked at your code.

Review status: 2 of 7 files reviewed at latest revision, 19 unresolved discussions.

go/vt/vttablet/heartbeat/heartbeat.go, line 1 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Please add a package comment here:
https://github.com/golang/go/wiki/CodeReviewComments#package-comments

Done.

go/vt/vttablet/heartbeat/heartbeat.go, line 12 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

Should vttablet record (if master) or check (if replica) replication heartbeat.

I would reformulate the beginning to make it easier to read and "parse" the sentence.
Additionally, you could briefly describe the feature in more detail?
Example:

If true, vttablet records (if master) or checks (if replica) the current time as replication heartbeat in the table _vt.heartbeat.

Done.

go/vt/vttablet/heartbeat/heartbeat.go, line 13 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

. Default 1s

No need to mention the default? It should already show up in the help text?

Done.

go/vt/vttablet/heartbeat/heartbeat.go, line 15 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Internally, we require that each monitoring variable is extensively documented.
Can you please add a comment here?

Done.

go/vt/vttablet/heartbeat/reporter.go, line 10 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Please change this to:
	log "github.com/golang/glog"
Our internal codebase depends on this when rewriting the import path.

Done.

go/vt/vttablet/heartbeat/reporter.go, line 20 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
nit: Missing period and throughout the file.
https://github.com/golang/go/wiki/CodeReviewComments#comment-sentences requires full sentences, including the "." :)

Done.

go/vt/vttablet/heartbeat/reporter.go, line 22 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
We assume that every field in the same group as mu must be guarded by it.
See also: https://talks.golang.org/2014/readability.slide#20
To comply with that, you need to move around things.
wg is probably not guarded but isMaster for example is.

Done.

go/vt/vttablet/heartbeat/reporter.go, line 41 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

from the watchHeartbeat routine

I find this statement misleading. How about "It returns the last reported value written by the watchHeartbeat goroutine"?
Maybe also add "If we're the master, " ?

Done.

go/vt/vttablet/heartbeat/reporter.go, line 58 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Write these as two defer statements instead?
defer r.wg.Done()
defer tabletenv.logError()

Done.

go/vt/vttablet/heartbeat/reporter.go, line 144 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
How about simplifying this and have only err as parameter?
You could use fmt.Errorf in the calling code to format your error message.
Then below, you would just call log.Error(err).

Done.

go/vt/vttablet/heartbeat/reporter.go, line 158 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Can you please move this to the top?
It's effectively the constructor of the object.

Done.

go/vt/vttablet/heartbeat/reporter.go, line 158 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
re: the return values. You're currently exposing an implementation detail. Instead, how about the following?
Export the type (reporter -> Reporter) and add an exported method Close which will call cancel() and wg.Wait(). Then, vttablet can call Close() on the returned object.

Done.

go/vt/vttablet/heartbeat/reporter.go, line 159 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

var wg sync.WaitGroup

wg := sync.WaitGroup{}
This way you can write wg instead of &wg below. Looks nicer.

Done.

go/vt/vttablet/heartbeat/reporter.go, line 161 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
I would return early here if there's nothing to do:
func RegisterReporter(...) {
if !*enableHeartbeat {
  return nil, nil
}

wg := sync.WaitGroup{}
...
return wg, cancel

This way you avoid indenting the code which is actually the main use case of this function.
This comment is similar to what Go prefers for error handling in general: https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow

Done.

go/vt/vttablet/heartbeat/writer.go, line 17 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

engine

It looks like you called this file engine.go before and renamed it then? How about removing the term "engine"? I'm concerned we're using it somewhere else as well and this would introduce confusion. "Writer" sounds like a look name to me? Maybe even rename the other class to Reader?

Done.

go/vt/vttablet/heartbeat/writer.go, line 77 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Maybe close the connection after Wait() returned to avoid spurious errors and have a more graceful shutdown?

Done.

go/vt/vttablet/heartbeat/writer.go, line 138 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Why not just checking if err != nil? I would prefer if we don't need the field inError.

I was trying to avoid spamming the logs with a bunch of repeat errors.  For brand new keyspaces, they don't have a master until one is elected, so all tablets will spam the errors. I'd be happy to take a different approach if you know of a better one.

go/vt/vttablet/tabletmanager/healthcheck.go, line 33 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
nit: Can you please move this to the previous group? Same comment for the other files.

Done.

go/vt/vttablet/tabletserver/tabletserver.go, line 450 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Please keep the return nil.
Otherwise, we could accidentally introduce a bug here: If you leave out the nil, the value of err is always returned. If err gets set but this method does not return immediately, you may end up returning it here although you meant return nil.
In general, I'm not a big fan of naked returns and named arguments. They only make sense for very short functions.

Done.

Comments from Reviewable

Thanks for the review @michael-berlin. I've responded to all of your comments. One thing I did differently than your suggestion is I decided to use the ThrottledLogger instead of my own if me.inError stuff. Let me know if you'd prefer something different, or if the documentation needs any further tweaks.
I still need to change to use a pool and follow the example of twopc. Will take care of that tomorrow.

Review status: 2 of 7 files reviewed at latest revision, 19 unresolved discussions.

Comments from Reviewable

Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

go/vt/vttablet/heartbeat/heartbeat.go, line 27 at r12 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Thanks for the comment! Very helpful.
After reading it, I suggest to split this into multiple variables instead. In general, a variables map like the Counters type should only group similar things. But I would argue that the errors count is different from e.g. the read count. Similarly, I feel like the read and write count are different and also the lag.
Each one could just be a stats.Int object instead.

I can do that. I was following the example of some of the other usages of Rates. I figure there must be a reason for these stat types.

go/vt/vttablet/heartbeat/heartbeat.go, line 34 at r12 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Computing rates in the code is usually not recommended.
This should be taken care of by the monitoring system.
Are you sure you need this?

I think the benefit to calculating in code is when your monitoring system cannot poll at a frequency fast enough to calculate the rates you want. For instance polling every 5 minutes can only calculate a 5 minute average. Making available the rates allows someone to choose the averages they want based on the interval they've chosen. I almost made the sampling interval configurable as well, but didn't want to be too confusing with multiple interval configurations.
All of that said, my monitoring system can poll rather quickly so I don't mind losing this myself.

go/vt/vttablet/heartbeat/reporter.go, line 28 at r12 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
How about renaming this to Reader and reader.go?
This way, the code in the package would look very symmetric :)
And after all, most of the code is for the reading part and not the reporting part.
Reporting is just a side job of the reader, isn't it? ;)

I took this a step further than you asked. I'm not crazy about exposing the Reader to the healthcheck system. In my opinion it's confusing that there would be a replication reporter and a heartbeat reader.
So I renamed to reader.go, and then moved the few healthcheck-related functions to a clean reporter.go.  RegisterReporter now creates a new Reader, Opens it, wraps it in a Reporter and returns. The Reporter handles interfacing with healthchecks.

go/vt/vttablet/heartbeat/reporter.go, line 124 at r12 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Move this if into watchHeartbeat?
if isMaster {
	if err := r.readHeartbeat(ctx); err != nil {
		r.recordError(err)
	}
}

This way, this method becomes more focused and you can avoid passing isMaster and the "unless.." part in the function name can be removed.

I was trying to do away with the multiple levels of indentation by moving it in there, but happy to do it that way if it's preferred :)

go/vt/vttablet/heartbeat/reporter.go, line 160 at r12 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
nit: missing period :)

This will take some getting used to. Punctuation? Gah! :)

go/vt/vttablet/heartbeat/reporter.go, line 168 at r12 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Can you not just write return conn.ExecuteFetch(...) ?
This would only be different if it returns a non-nil response if err != nil. I don't know if that's the case.

Done.

go/vt/vttablet/heartbeat/writer.go, line 138 at r6 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Okay, I understand now :) I think a different variable name would have made it easier to understand the purpose e.g. for the reader such a boolean could have been named "missingRowLogged" or "waitingForRow".
But I think throttled logging is fine here and the most generic solution. No need for more custom solutions.
In general:
The writer itself should rarely fail and log an error, shouldn't it? If it does, only because during a reparent MySQL has already become readonly but vttablet still thinks we're the master?
For the reader I agree that there can be a longer period of time where the row does not exist.

You're right the Writer is less susceptible here. I'd like to keep it though because while it is less susceptible, it's not immune. Something could happen to cause writes to fail -- such as the server going into read only mode (which I've seen), or someone messing with the heartbeat table, or someone changing the master uid. In all of those cases, the read only being the more realistic, the thread will spam many logs per second depending on the configured interval.

go/vt/vttablet/heartbeat/writer.go, line 33 at r12 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
I see that it's also defined in connpool/pool.go.
@sougou can we just export connpool.mySQLChecker and this way avoid repeating the interface definition here?

I'd be happy to do this if desired. There is also the source MySQLChecker in tabletenv/config.go. I think the reason why this is repeatedly defined in a few places is to avoid cyclic dependencies. If we can choose one which is unlikely to cause that, I could change it.

go/vt/vttablet/heartbeat/writer.go, line 40 at r12 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Please regroup the fields under mu here as you did for the reader.

Done.

go/vt/vttablet/heartbeat/writer.go, line 44 at r12 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
You can delete the inError field from the Writer now.

Done.

go/vt/vttablet/heartbeat/writer.go, line 61 at r12 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
optional comment: Very Java-esque formatting here. In Go, we would probably just squeeze everything into one line. I'm fine with both styles, so no need to change here.

Done.

go/vt/vttablet/heartbeat/writer.go, line 69 at r12 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

connections

"connection" because there's only one?
(Same comment below for Close().)

Done.

go/vt/vttablet/heartbeat/writer.go, line 130 at r12 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
nit: missing period :)

Done.

go/vt/vttablet/heartbeat/writer.go, line 146 at r12 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
nit: missing period :)

Done.

Comments from Reviewable

Review status: all files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

go/vt/vttablet/heartbeat/heartbeat.go, line 31 at r12 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
nit: missing period :)

Done.

Comments from Reviewable

Review status: 3 of 9 files reviewed at latest revision, 9 unresolved discussions.

go/vt/vttablet/heartbeat/heartbeat.go, line 32 at r13 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
How about splitting this into two variables i.e. readErrors and writeErrors?
I think there's value in that and it shouldn't complicate things much?

Done.

go/vt/vttablet/heartbeat/reader.go, line 25 at r13 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

isreported

typo: is reported

Done.

go/vt/vttablet/heartbeat/reader.go, line 56 at r13 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
nit: Missing period. duck

Done.

go/vt/vttablet/heartbeat/reader.go, line 57 at r13 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Reader.Open() is different from Writer.Open(): A Writer can be opened and closed repeatedly. Instead, a Reader is a onetime use object.
In order to avoid programming errors here, I suggest that you add an assertion-like check in the beginning. For that, we generally use log.Fatalf with the message prefix BUG: .
Example:
if r.wg != nil {
  log.Fatalf("BUG: Reader object cannot be initialized twice: %v", r)
}

(Reader.Close() seems to be idempotent as is. I would just keep it this way.)
The alternative to this check in Open() would be to move the init code back into the constructor and have no Open(). I think I would prefer this but both approaches work for me.

What if I change Reader to also check isOpen like Writer does? I think I agree that keeping the constructor lightweight and launching the goroutine in Open() seems preferable

go/vt/vttablet/heartbeat/reporter.go, line 34 at r13 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

Reader

Reporter

Done.

go/vt/vttablet/heartbeat/reporter.go, line 39 at r13 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

Reader

Reporter

Done.

go/vt/vttablet/heartbeat/reporter.go, line 45 at r13 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
I think you should move this code into a method of Reader. This way the two classes are better isolated and only Reader methods use the mutex.

Done.

Comments from Reviewable

Added some tests for the writer. Reader tests still in progress.

Review status: 3 of 9 files reviewed at latest revision, 9 unresolved discussions.

Comments from Reviewable

@michael-berlin I changed the reader Open/Close to work like the writer. I think that's cleaner than throwing an exception because I was trying to make this as side-effect proof as possible. I also renamed HeartbeatLagNs to HeartbeatLagNsCount, so it matches the others and so that it's more obvious that it is not a point-in-time measurement of lag.
Thanks for the review work. It was very helpful in learning how to write cleaner go code, and this is looking much better than when I first started. The links were helpful, I'll read over those in full.
Tests are now there. This is ready for review from @sougou when he has a moment.

Review status: 8 of 10 files reviewed at latest revision, 20 unresolved discussions.

go/vt/vttablet/heartbeat/heartbeat.go, line 28 at r16 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
I actually liked the old names better. Having Count both for the variable itself and the stats name feels redundant?
reads, writes, readErrors, writeErrors should be unambiguous?

Done.

go/vt/vttablet/heartbeat/heartbeat.go, line 37 at r18 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

HeartbeatLagNsCount

How about renaming this to HeartbeatCumulativeLagNs ? I think it's important to stress that it's a cumulative count. By default, people would probably read "lag" and assume that it's a gauge metric.

Done.

go/vt/vttablet/heartbeat/reader.go, line 57 at r13 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
I like that Reader is symmetric to Writer now. But on the other hand now Reader can be reused although it never will be in practice. In general, I prefer not to implement something "because you may need it some day".
However, for now let's keep Open/Close in the Reader object.

Done.

go/vt/vttablet/heartbeat/reader.go, line 69 at r18 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
I think calling Open() again is a programming error and we should have log.Fatalf("BUG: ...") here. Same for the Writer object.
btw: If you would ask Alain, he would say that this shouldn't crash vttablet and instead only log something. That's because he's concerned about the stability of vttablet and doesn't want that something random can bring it down. But for this usecase, I disagree. You want to find out about these problems as early as possible and therefore we should log.Fatalf here :)
Same comment for the Writer.
Close below is idempotent. That can be kept as is.

I implemented the BUG fatal into Reader, but slightly different for Writer. Writer actually will be opened multiple times, but it should be closed in between. Reader should only ever be called once.
I called this out in the comments of each function

go/vt/vttablet/heartbeat/reader.go, line 76 at r18 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Please "align" this code with Writer.Open() i.e. make it as similar as possible.
also: "wg" can be a non-pointer type and no longer needs to be initialized explicitly (In your test you need to stop setting it as well.)

Done.

go/vt/vttablet/heartbeat/reader_test.go, line 42 at r18 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
optional: I would avoid the intermediate variable here and just inline the call in the function call on the next line.

Done.

go/vt/vttablet/heartbeat/reader_test.go, line 44 at r18 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
I would remove this newline.

Done.

go/vt/vttablet/heartbeat/reader_test.go, line 63 at r18 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
nit: missing period

Done.

go/vt/vttablet/heartbeat/reader_test.go, line 83 at r18 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
No need to wrap the constant in time.Duration()?

Done. Though & must point at something addressable, so had to do it in 2 calls

go/vt/vttablet/heartbeat/reader_test.go, line 93 at r18 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
This is sensitive to timings as well and not going to work if the system is under load.
These three points assume a synchronization which you currently try to enforce with the sleeps:
a) You don't want to start rejecting the query before the Reader processed the event and set isMaster = true.
b) When you set lagAtSwitch, you assume that the loop has stopped. But this is not guaranteed.
c) After you started rejecting the query, you don't want to cancel the loop too early. It should go through at least once.
Given all that, I suggest the following changes:

Don't call the loop watchHeartbeat() at all and instead call only readHeartbeat().
move the logic for setting and checking isMaster into readHeartbeat().

Now you can write a test which has the following flow:

add the event listener (you'll probably want to move that into a method and call that method)
run readHeartbeat() and check the state
dispatch the event
wait for the state to change: You could add a function isMaster() and constantly poll that. Once it's true, you can continue.
run readHeartbeat() again and make sure that lagNsCount didn't increase

Done.
One more concern along the lines of concurrency is that some of the tests rely on the counters, which are global. I had to explicitly set them to 0 at the beginning of tests that care about the values. Are tests run in parallel in travis? If so things may collide.

go/vt/vttablet/heartbeat/reader_test.go, line 144 at r18 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

t.Errorf

These should probably be t.Fatalf and not Errorf. Only fatal will fail the test immediately while error will continue with the test execution. We use Errorf very seldom.

Done.

go/vt/vttablet/heartbeat/reader_test.go, line 154 at r18 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Remove this because it's not necessary?

This is necessary to force a call to GetShard. Added a comment.

go/vt/vttablet/heartbeat/reader_test.go, line 158 at r18 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Please test the error message as well. Otherwise, the code may fail for the wrong reason.

Done. Though wasn't totally sure of the idiomatic way to do so. Seems not great to copy/paste the format string, but also seemed like overkill to add a var or function to generate it

go/vt/vttablet/heartbeat/reader_test.go, line 161 at r18 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Your "want" value is different between here and your error message.
To avoid this, you can use this Go idiomatic style:
if got, want := r.lastKnownMaster, 1111; got != want {
  t.Errorf("wrong lastKnownMaster: got = %v, want = %v", got, want)
}

The error message was wrong, the want was right. I changed to 3333 and used your idiomatic style

go/vt/vttablet/heartbeat/reader_test.go, line 166 at r18 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
You can just inline this?

Done.

go/vt/vttablet/heartbeat/writer_test.go, line 36 at r16 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
The test is missing a comment which says that waitForTables is failing and retrying because the CREATA DATABASE SQL is not registered with the fake. It took me a bit to figure this out :)

Done.

go/vt/vttablet/heartbeat/writer_test.go, line 45 at r16 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Coming back to this after reviewing reader_test.go: The alternative here would be to not test the loop in the first place. This would give you more control and the test code would become simpler. You should consider doing that.
In that case, you can basicall fold waitForTables into run. The other nice thing of that would be that the init tables for loop happens at the same level as the write for loop.

Done. This loses the test for retries, but perhaps that part is not as important as executing the right queries.

go/vt/vttablet/heartbeat/writer_test.go, line 50 at r16 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
You can remove this because you're already calling it above.

Done.

go/vt/vttablet/heartbeat/writer_test.go, line 111 at r16 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Remove this because Open() does set it.

Actually I'm explicitly not calling Open(), because it kicks off the goroutine which interferes with the per-function tests

go/vt/vttablet/tabletserver/tabletserver.go, line 53 at r18 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Please move this one group as well.

Done.

Comments from Reviewable

All comments addressed, including changing of MySQLChecker and removing of the MasterUid sanity checks.

Review status: 4 of 10 files reviewed at latest revision, 13 unresolved discussions.

go/vt/vttablet/heartbeat/heartbeat.go, line 28 at r19 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

HeartbeatWriteCount

Revert this back to HeartbeatWrites as well? What do you think? It would be less verbose and still clear?
Similarly, you could name the other variables HeartbeatReadErrors etc.?

Done.

go/vt/vttablet/heartbeat/reader.go, line 9 at r19 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Must be:
log "github.com/golang/glog"

Done.

go/vt/vttablet/heartbeat/reader.go, line 76 at r19 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Let's not use a pointer type for wg. Your initial Reader code needed it because you returned it. Now it's only used within the each Reader and Writer.
Therefore, please change wg to the non-pointer type (is there a better word to express this?) in both Reader and Writer: sync.WaitGroup.
This way you can use the zero value of the field and you don't need to initialize it in Open().

Done. I added an isOpen to Reader in this case to verify that Open is not called multiple times.

go/vt/vttablet/heartbeat/reader.go, line 152 at r19 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
I've thought about this check more and I think we should not do it.
Other code in Vitess which does similar things does not rely on the topology as well. By not querying the topology, you also avoid that X shards * Y replicas are hammering the topology every second.
If you're concerned about breaking ties between two masters which seem to be running at the same time, you should add a column "last_reparented_timestamp" and update it in the writer.
The ActionAgent has this information: https://github.com/youtube/vitess/blob/master/go/vt/vttablet/tabletmanager/rpc_external_reparent.go#L214
Currently, there's no exported method to retrieve this information. But it would be easy to add. The name of the field is misleading because it has "external" in it for historical reasons. Initially, it was only set for external reparents. But I've changed it a while ago to be set for any observed reparent.
Sorry for not catching this earlier in the review. This might be a more involved change but not too complex?
More generally, which other problems are you trying to solve here? Maybe I am missing something? Feel free to ping me on Slack about that.
I could think of weird split brain situations where you have two partitions with a MySQL master in each. (IMO that's something which should never happen in the first place and should be caught by a system which monitors the MySQL replication. But that's probably not available today?) If the topology was not partitioned as well or reads to it are failing, then this check would help.
But I think there's not much to gain from such a functionality? Breaking ties via the reparented timestamp should be sufficient for most cases?

Agreed. This was an evolution of features that led to this, and perhaps it's not necesary. The alternative I can think of is to call GetShard on StateChange events. That would reduce the calls to topology, allow us to be more specific with our SELECT query, and avoid the need for checking.
For now will leave it out though.

go/vt/vttablet/heartbeat/reader_test.go, line 158 at r18 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Yes.
Please don't repeat the format string in the test. Instead, we generally write the wanted error message as string literal in the test. Often, it's also sufficient to do a partial test (strings.Contains(err.Error(), want)) if the error message has dynamic data that changes with each test.
In general, Go is against checking error messages because it's a pain to change the test when you change the message. Instead, you're supposed to test behavior instead. But I don't see a way how we could do this here. Checking the error is the right thing to do here.

Done.

go/vt/vttablet/heartbeat/reader_test.go, line 108 at r19 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Using Context is doable but looks more heavy.
Other Vitess code uses this simpler pattern:
start := time.Now()
for {
	// Do stuff.

	if time.Since(start) > 10*time.Second {
		t.Fatal("Reader did not update its isMaster state after event dispatch")
	}
	time.Sleep(1 * time.Millisecond)
}

Note that we're calling Sleep at the end to avoid a tight loop. Some of my code didn't have it and Sugu called this out. Personally, I wouldn't care about that because it's test code. But let's be consistent here :)

Done.

go/vt/vttablet/heartbeat/reader_test.go, line 110 at r19 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Sorry, I think my earlier comment confused you. I meant you could add a isMaster() method to Reader and then call that
a) here:
if r.isMaster() {
  break
}

b) and in readHeartbeat() as well:
if r.isMaster() {
  return nil
}

:man_facepalming:  that makes sense. Done.

go/vt/vttablet/heartbeat/reader_test.go, line 130 at r19 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

want.Nanoseconds()

nit: got and want should be of the same type. This check should always read got != want.
You could assign 20*time.Second.Nanoseconds() to want instead to fix this.

Done.

go/vt/vttablet/heartbeat/reader_test.go, line 177 at r19 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
I think you should not change the internal state of the object and instead use the external API of it as much as possible.
What you actually want is that the query returns a different UID, right?
That should be easy to achieve. You can manipulate the result returned by the fake db:
result = db.AddQuery(...)

// Initial read.

// Change UID and re-run read.
result.Rows[0][1] = sqltypes.MakeTrusted(...)

See also my other comment about querying the topology. This test may no longer be necessary after that.

Done.

go/vt/vttablet/heartbeat/writer.go, line 33 at r12 (raw file):
Previously, sougou-bot (Sugu Sougoumarane) wrote…
I had them separate because I was anticipating that each package would require different things from TabletServer. This would have been the case if TxEngine were to become its own package. If what we have now is all common, I see no harm in moving it to a common location, and connpool seems to be the best place because it's the on requesting it at the lowest level.

Ok, I pushed a revision which exposes connpool's MySQLChecker and utilizes that where I was able to find individually declared versions.

go/vt/vttablet/heartbeat/writer.go, line 26 at r19 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Can you please reorder the columns and list the primary key first?

Done.

go/vt/vttablet/heartbeat/writer_test.go, line 45 at r16 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
You could still test that the init tables code is failing if the statements don't succeed.
e.g. you could change the beginning of this test as follows:
// initializeTables fails if it cannot run any of the SQL statements successfully.
err := te.initializeTables(db.ConnParams())
if err == nil {
		t.Fatal("initializeTables() should not have succeeded")
}
// TODO: Check err in more detail as well.

// Register queries and let initializeTables succeed.
db.AddQuery(fmt.Sprintf(sqlCreateSidecarDB, te.dbName), &sqltypes.Result{})

I thought about this and didn't deem it worth it, but if you also thought about it it's cheap enough to do. Done.

go/vt/vttablet/heartbeat/writer_test.go, line 44 at r19 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

numWrites, want

got, want to be consistent.

Done.

Comments from Reviewable

Thanks for the detailed review! I learned a lot

Review status: 9 of 16 files reviewed at latest revision, 4 unresolved discussions.

go/vt/vttablet/heartbeat/reader.go, line 152 at r19 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
re: the StateChange events. A replica would see only its own events, not the ones from the other masters. That's why this should not work.
The alternative to polling via GetShard would be to do what vtgate does: Use the topology to find out about all tablets within a shard and then connect to them. Once connected, you would receive of the constant stream of health updates (StreamHealthResponse) which includes the tablet type. The code for that is in the "discovery" package and vtworker for example uses it to find the current master. But between this and writing the last reparented timestamp in a new column, I would prefer the latter.
But I think this version will be fine and we should not add any of this. If it turns out we're wrong, it would be easy to extend :)

Thanks for the context there. It's helpful to know how that would work

go/vt/vttablet/heartbeat/reader.go, line 177 at r21 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

master uid values

Please update the comment here.

Done.

go/vt/vttablet/heartbeat/reader.go, line 24 at r22 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

, master_uid

Remove this from the query since you're not using it anymore.

Done.

go/vt/vttablet/heartbeat/reader.go, line 34 at r22 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
Remove this field here and in the tests.

Done.

Comments from Reviewable

Doing some testing in my env. But addressed all comments.

Review status: 3 of 15 files reviewed at latest revision, 7 unresolved discussions.

go/vt/vttablet/heartbeat/heartbeat.go, line 25 at r23 (raw file):
Previously, sougou-bot (Sugu Sougoumarane) wrote…
With the model where TabletServer decides if a service should or should not run, these two may have to be moved to tabletenv.Config.

Done.

go/vt/vttablet/heartbeat/reader.go, line 39 at r23 (raw file):
Previously, sougou-bot (Sugu Sougoumarane) wrote…
Other users of ticker suffer from the same disadvantage. For the sake of uniformity, I'd still prefer using ticker. What we can later do is to upgrade ticker to use context. That way, everyone will benefit uniformly.

Done.

go/vt/vttablet/heartbeat/reader.go, line 133 at r23 (raw file):
Previously, sougou-bot (Sugu Sougoumarane) wrote…
You shouldn't have to check isMaster at this level. TabletServer makes the decision on which service should and should not run based on its criteria. Reader should just run if it was 'Opened'. TabletServer will also need to 'Close' the reader if it becomes master. You can see how te.Open and Close are called in TabletServer.

Done.

go/vt/vttablet/heartbeat/reporter.go, line 22 at r23 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…

topoServer topo.Server

This can be removed as well.

Done.

go/vt/vttablet/heartbeat/writer.go, line 137 at r23 (raw file):
Previously, sougou-bot (Sugu Sougoumarane) wrote…
This should ideally be an exported Init function that tabletserver would call on fullStart, just like it does for TxEngine. This way, the tables will get created only on vttablet startup, and not on every state change.

Done.

go/vt/vttablet/tabletmanager/healthcheck.go, line 132 at r23 (raw file):
Previously, sougou-bot (Sugu Sougoumarane) wrote…
Based on my understanding, it looks like there's not much benefit to tabletmanager managing the life-cycle of the reporter. I think there will be eventual value if heartbeat replaced the older replication reporter. But until we cross that bridge, it will be simpler if TabeltServer took over the life-cycle of Writer, Reader and Reporter.
This should also lead to further simplification because the low level services won't have to check isMaster, tabletType, etc.

Done.

Comments from Reviewable

Review status: 2 of 15 files reviewed at latest revision, 8 unresolved discussions.

go/vt/vttablet/heartbeat/reporter.go, line 21 at r25 (raw file):
Previously, sougou-bot (Sugu Sougoumarane) wrote…
Final nit: The should be renamed to NewReporter and it should not check enableHeartbeat or Open itself. TabletServer.serveNewType should instead make those decisions.

I beat you to the punch on removing Open, but I wonder about checking enableHeartbeat.
If we don't check enableHeartbeat here, we'll register the healthcheck reporter. So people will see in the UI "Polling health information from MySQLHeartbeat  +  MySQLReplicationLag." even if they don't have it enabled.
If you think that's fine, then I'm fine with it. In fact, if we do that we can probably move the Reporter into the healthcheck package. If we create a tsv GetHeartbeat function, we could call that from within agent/healthcheck. This would make the reader and writer more symmetric within tsv.
Thoughts?

Comments from Reviewable

Review status: 2 of 15 files reviewed at latest revision, 8 unresolved discussions.

go/vt/vttablet/heartbeat/reporter.go, line 21 at r25 (raw file):
Previously, bbeaudreault (Bryan Beaudreault) wrote…
I beat you to the punch on removing Open, but I wonder about checking enableHeartbeat.
If we don't check enableHeartbeat here, we'll register the healthcheck reporter. So people will see in the UI "Polling health information from MySQLHeartbeat  +  MySQLReplicationLag." even if they don't have it enabled.
If you think that's fine, then I'm fine with it. In fact, if we do that we can probably move the Reporter into the healthcheck package. If we create a tsv GetHeartbeat function, we could call that from within agent/healthcheck. This would make the reader and writer more symmetric within tsv.
Thoughts?

I like the idea of heartbeat being a TSV thing, and keeping the healthcheck registration in 1 place. Making a NewReader function magically register a healthcheck seems a bit like action-at-a-distance.  So I'd propose something like:

Move reporter.go into tabletmanager. Change RegisterReporter to take a heartbeat.Reader.
Call RegisterReporter from healthcheck.go
Have TSV call the existing NewReader function in reader.go
Add a GetHeartbeatReader() or similar to tabletserver.Controller interface.  In healthcheck.go, call RegisterReporter(agent.QueryServiceControl.GetHeartbeatReader()) which returns a pointer to the reader for use in the newly very simple tabletmanager/heartbeat_reporter.go

Thoughts?

Comments from Reviewable

Review status: 2 of 18 files reviewed at latest revision, 9 unresolved discussions.

a discussion (no related file):
@sougou as we discussed I added a keyspace:shard column as the primary key in the table, and converted to using primary key-based reads and writes.
I decided to keep the masterUid (renamed to tabletUid) as well. While we don't use this in the heartbeat code, I think it is valuable from a debugging standpoint. Being able to go into mysql and see what tabletUid is being used is good, especially since for now every tablet writes its own initial heartbeat with the binlog off. I think it's worth the small handful of lines of code to pass in the alias.
In terms of keyspaceShard, it added a little ugliness -- I converted to using bindvars because I wasn't sure if we can trust the keyspace and shard values. I realize we're just protected ourselves from ourselves, but I figured there could be an accident that could cause, for instance a quote in the keyspace name or shard. I wasn't able to figure out if we validate keyspace/shard anywhere in the startup/registration phase. If we do or if this is overly paranoid, let me know and I can clean up the code a bit.

go/vt/vttablet/heartbeat/heartbeat.go, line 25 at r23 (raw file):
Previously, sougou-bot (Sugu Sougoumarane) wrote…
I think it will work out better if we moved it to tabletenv.Config, but not too sure. It depends on how the integration between Reporter and Reader works out. We'll see how that turns up.

Done. I moved the config into the tabletenv.Config as part of the Reporter/Reader refactor.

go/vt/vttablet/heartbeat/writer.go, line 59 at r26 (raw file):
Previously, sougou-bot (Sugu Sougoumarane) wrote…
I had a chat about this with @bbeaudreault on slack. For now, he's going to change this part to turn off binlogs just like 2PC. Ideally, we should create these only on Master (even the 2PC tables should be created like that). But there was a bug that prevented this because there are some workflows where vttablet master comes up without a replica, and these DDLs end up hanging waiting for semi-sync ack. We can change to the master-only behavior after the bug is fixed (or we confirm that it's fixed). @alainjobart was supposed to, but I didn't follow up on it.

Done.

go/vt/vttablet/tabletmanager/heartbeat_reporter.go, line 21 at r25 (raw file):
Previously, mberlin-bot (Michael Berlin) wrote…
SGTM :)

Done.
I think this is cleaner now.

Comments from Reviewable

Thanks guys :)


Troubles running 'remap_labels' on ProjectDataset

In this example, the dataset given from project.make_dataset() returns a ProjectDataset, I am not able to run remap_labels on this dataset as it just returns a TypeError: 'str' object is not callable. I looked through the tests and it looks like all the tests are running remap_labels on datumaro.components.dataset.Dataset objects and I don't see any tests/examples running on datumaro.components.project.ProjectDataset.
I'm not sure if I'm doing something wrong, but just running this example on the master and development branch seems to give me the same TypeError: 'str' object is not callable error.
If I use the dataset.Dataset class it works fine, but I'm not sure how to take a ProjectDataset and turn it into just a Dataset
from datumaro.components.project import Project

# load a Datumaro project
project = Project.load('directory')

# create a dataset
dataset = project.make_dataset()

# keep only annotated images
dataset.select(lambda item: len(item.annotations) != 0)

# change dataset labels
dataset.transform('remap_labels',
  {'cat': 'dog', # rename cat to dog
    'truck': 'car', # rename truck to car
    'person': '', # remove this label
  }, default='delete') # remove everything else

As a workaround I am running this instead
import datumaro.plugins.transforms as transforms

dataset = transforms.RemapLabels(dataset,
  mapping={'cat': 'dog', # rename cat to dog
    'truck': 'car', # rename truck to car
    'person': '', # remove this label
  }, default='delete')

This seems to get me past that step, but it changes the type of the dataset to a datumaro.plugins.transforms.RemapLabels

Alright, well that bombed when I tried to export the dataset. I'll keep looking for a solution. Any help on what I'm doing wrong or what I could correct would be great. Thanks!

Hi, thanks for reporting the problem with the example. Yes, it looks like ProjectDataset was not updated after recent Dataset changes and it can accept only a transform class instead of allowing a string name in the transform method. The workaround you tried was correct, it just represents how it works under the hood. To make everything working do any of the following:
# Just pass the transform class in the example code instead of string
dataset.transform(project.env.transforms['remap_labels'],

# or
from datumaro.plugins.transforms import RemapLabels
dataset.transform(RemapLabels,

# or continue the workaround you tried with
dataset = Dataset.from_extractors(dataset)
dataset.export('path/', 'format_name')

# or (answering your question about getting a Dataset from ProjectDataset)
dataset = Dataset.from_extractors(project.make_dataset())
... # the example code

All of this will produce the same results.


Unable to load an HTTPS URL with static binary

Hello !
I have a castor application that sent HTTPS requests to load files.
It works perfectly well with the castor version or the phar version.
But the linux version encounters an error.
The binary was compiled with the openssl extension (default value)
file_get_contents('https://[...]');

Warning: file_get_contents(): SSL operation failed with code 5. OpenSSL Error messages:
error:80000002:system library::No such file or directory
error:80000002:system library::No such file or directory
error:80000002:system library::No such file or directory
error:0A000086:SSL routines::certificate verify failed

I have implemented a workaround at PHP level but it is not ideal
file_get_contents('https://[...]', context: stream_context_create([
    "ssl" => [
        "verify_peer"      => false,
        "verify_peer_name" => false,
    ],
]));

Is the URL Public so we can test? Or does it work with GitHub.com for example?
If you have SSL error you can also use the http client with some options to disable peer verification

Yes
#[AsTask(description: 'https://github.com/jolicode/castor/issues/430')]
function issue430()
{
    file_get_contents('https://github.com/jolicode/castor/blob/main/composer.json');
}

it works with Castor but it doesn't work with binary

Hello, I tried the following setip

download a static binary: https://castor.jolicode.com/getting-started/installation/#as-a-static-binary
Create a new castor.php with the following content:<?php

use Castor\Attribute\AsTask;

use function Castor\io;
use function Castor\request;

#[AsTask(description: 'Make HTTP request')]
function httpRequest(string $url = 'https://example.com'): void
{
    $response = request('GET', $url);

    io()->writeln($response->getContent());
}

Execute it:$ ./castor-static  http | head
<!doctype html>
<html>
<head>
    <title>Example Domain</title>

    <meta charset="utf-8" />
    <meta http-equiv="Content-type" content="text/html; charset=utf-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1" />
    <style type="text/css">
    body {

$ ./castor-static  http --url  "https://github.com" | head

<!DOCTYPE html>
<html
  lang="en"

ensure I got the static binary$ file ./castor-static
./castor-static: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), statically linked, stripped

Everything works as expected. what did I miss?

For the record, I also repack + compile my app, end it still work
$ /tmp/foobar/castor-ssl/my-app.linux.x86_64 http
<!doctype html>
<html>
<head>
    <title>Example Domain</title>

    <meta charset="utf-8" />
    <meta http-equiv="Content-type" content="text/html; charset=utf-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1" />
    <style type="text/css">
    body {
        background-color: #f0f0f2;
        margin: 0;
        padding: 0;
...

And I even try in an alpine container:
$ docker run -it  -v $(pwd):/app --rm alpine  /app/my-app.linux.x86_64 http
<!doctype html>
<html>
<head>
    <title>Example Domain</title>

    <meta charset="utf-8" />
    <meta http-equiv="Content-type" content="text/html; charset=utf-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1" />
    <style type="text/css">
    body {
        background-color: #f0f0f2;
        margin: 0;
        padding: 0;
        font-family: -apple-system, system-ui, BlinkMacSystemFont, "Segoe UI", "Open Sans", "Helvetica Neue", Helvetica, Arial, sans-serif;

    }

Thanks a lot !
There is no problem with request()
// ☑
$response = request('GET', 'https://github.com/jolicode/castor/blob/main/composer.json');
$json = $response->getContent();

But there is a problem with file_get_contents()
// 🚫
$json = file_get_contents('https://github.com/jolicode/castor/blob/main/composer.json');

I'm going to use request() for now but there is a problem with certificates if we directly use a native PHP function.

Indeed, I don't know why. But since there is a much better way to do, let's close this for now.
Thanks for the issue!

This thread documents this issue 👍


Data analysis

As a
user
I want to
see what learning technique I excel in and I struggle with.
To
track my progress

Acceptance Criteria

[x] working data analytics page
[x] dialog that shows the user's strength, and suggests other learning techniques, this can be shown at intervals so that the user won't get annoyed every time he opens the data analytics fragment

:tada: This issue has been resolved in version 1.0.0 :tada:
The release is available on GitHub release
Your semantic-release bot :package::rocket:


Provide a way to not use the source directory for relativeTo

@shinnn In old versions of broccoli-clean-css relativeTo can be set to anything. However in newer versions it is always relative to the source directory.
Libraries that use the old behaviour are blocked from upgrading because of this (ember-cli/ember-cli-preprocessor-registry#6)
Can we get some way to bypass this forced rebasing?

I believe that the behavior of relativeTo in broccoli-clean-css is correct even though it was a breaking change to ember-cli (which was absolutely fine to do in a major version bump). See my detailed explanation in https://github.com/ember-cli/ember-cli-preprocessor-registry/issues/6#issuecomment-183884529.
ember-cli is being updated to use the correct value for relativeTo in https://github.com/ember-cli/ember-cli/pull/5463.

Cool! There is no problem then.


General improvements

Improved types and also use more types in test data
Fixed test npm run build:test:girs
Upgrade dependencies (With the upgrade of eslint some new automatic formatting has been added which increases the PR)
Added tests to README.md
Added version to fix #25
Added src to NPM module to fix #20
Split method exportObjectInternal in more methods to make it more readable
Improved debugging by log the full error when an error occurs
Some improvements taken from @realh `s fork

@realh If you are reading this: I hope we can bring your fork closer together because it looks like that you make some very useful improvements. The src/gir-module.ts is similar to the main.ts in your fork

I did get notified about this. I should really have done a PR ages ago, but I thought I should test my version more thoroughly before getting you to deal with such intrusive changes. But then I got sidetracked on other projects and only recently started using ts-for-gjs regularly again. I've just fixed some other problems in my fork today. I'll try to somehow make it possible to merge my branch, but unfortunately I didn't realise until today that the source code had been split up, while I've still been using the monolithic main.ts. That will make things difficult for me, but it is a welcome change.

@realh Yes I think to merge your fork normally has become almost impossible. But maybe you can start to implement your ideas in a new and clean way. I have started to unify some methods with your methods which should make it a bit easier for you.
Maybe we can introduce a new CLI flag experimental-inheritance to use your experimental inheritance feature?

A large review this one, but looks really good!


Add analytics_anonymize_ip option to theme

To easy usage according to the GDPR I added an option to enable anonymization of ip addresses when using Google Analytics

Any further feedback?


Level INF (uppercase) not parsed correctly

Hello,
First, thanks for the great work!
While trying to tweak the config file, I realized that the info level does not parse INF correctly. Lowercase inf works fine. You can try with the attached files.
I'm on Windows with v0.29.5.
Thanks,
Laurent
example.log
example-hl-config.yaml.txt

Same with v0.29.6. Thanks.

Created issue #568 for the config-rs crate.
This problem seems to be specific to the YAML format, because in most other formats, strings must be enclosed in quotes.
I am thinking about ignoring case for levels. This would not only work around the problem, but also make it easier to configure for different spellings.

Thanks for the quick fix and the workaround. 👍


Register htmlbars inline precompile on registry

This refactors the way how the HTMLBarsInlinePrecompilePlugin is
registered on the babel pipeline: previously the ember-cli-babel options
were hijacked and the babel plugin has been added.
Since ember-cli-babel v5.X.X registered addons under the babel-plugin
type are added automatically. Since ember-cli-htmlbars v0.7.7 a
precompile method is exported on the plugin, so there is no more need
to manually get the HTMLBarsCompiler out of bower_components.

This addresses #7 and closes #4 and #5.

:+1: - LGTM

@rwjblue thanks for reviewing!