Work in progress - switch to eventstreaming

[marcbowes]

No description provided.

[andrewt-amzn]

Some superficial comments and questions mostly on the builder. I will take a second pass at grokking the session pool management and error handling later.

[andrewt-amzn]

Is there a way to alias the import to make that clear in the code? Or is the generic sounding Config type just something to get used to?

[marcbowes]

I'm not sure, this is a newly introduced change in the SDK. I think there might be something clever I can do with generics to make it so there is only 1 API that can accept either.

[andrewt-amzn]

Why do these functions need to be async?

[marcbowes]

Setting up the bb8 pool launches a background coroutine that manages connections. In Kotlin speak, this needs to be in the same coroutine context, and thus would have been modeled as an extension function.

One nice thing about Rust coroutines vs Kotlin coroutines is all the await points are obvious:

    pub async fn config(self, config: Config) -> QldbResult<QldbDriver<DynConnector>> {
        let client = Client::from_conf(config);
        self.build_with_client(client).await
                                    // ^^^^^
    }

    pub async fn build_with_client<C>(self, client: Client<C>) -> QldbResult<QldbDriver<C>>
    where
        C: SmithyConnector,
    {
        let ledger_name = self
            .ledger_name
            .ok_or(error::usage_error("ledger_name must be initialized"))?;

        let transaction_retry_policy = Arc::new(Mutex::new(self.transaction_retry_policy));

        let session_pool = Pool::builder()
            .test_on_check_out(false)
            .max_lifetime(None)
            .max_size(self.max_concurrent_transactions)
            .connection_timeout(Duration::from_secs(10))
            .error_sink(Box::new(QldbErrorLoggingErrorSink::new()))
            .build(QldbSessionV2Manager::new(client, ledger_name.clone()))
            .await // <-------
         // ^^^^^
            .map_err(|_| error::todo_stable_error_api())?;

        Ok(QldbDriver {
            ledger_name: Arc::new(ledger_name.clone()),
            session_pool: Arc::new(session_pool),
            transaction_retry_policy,
        })
    }

[andrewt-amzn]

The bb8 doc says that the future completes when the pool has the requested number of connections open. Since you're not specifying min_idle, we will have a pool with zero sessions initially, right?

[marcbowes]

I can't remember now, but yeah I think so.

I was thinking about making APIs that mirror the bb8 pool (i.e. exposing min_idle on our builder), but I'm not sure. Got any thoughts?

[andrewt-amzn]

This is mapping the error variant from bb8 Builder.build, not an aws sdk error, right?

And map_err results in another Result, rather than the success variant of the prior Result rather than being some macro which can return an error? I'm a bit lost about how we get from here to building the Ok result with a concrete session_pool.

[marcbowes]

Yeah. So..

pub async fn build(self, manager: M) -> Result<Pool<M>, M::Error> {

The error variant is the associated Error type, i.e. this one:

#[async_trait]
impl<C> ManageConnection for QldbSessionV2Manager<C>
where
    C: SmithyConnector,
{
    type Connection = QldbHttp2Connection;
    type Error = ConnectionError;

and ConnectionError is defined by us to be:

pub type ConnectionError = SdkError<SendCommandError>;

So it's just the underlying SdkError that represents the issue - connection, credentials, invalid endpoint etc.

Previously, I had the carrier infrastructure (? operator) turn that into QldbError::SdkError. As per our discussion last week, you convinced me to abstract the SDK errors away. So I just put a todo there to remind myself.

map_err is a function that turns Result<T, E1> to Result<T, E2> (as you said). So it doesn't mess with the pool at all, just the error. The T itself is the pool, and the carrier trait essentially expands to:

let pool = match stuff() {
  Ok(it) => it,
  Err(err) => return Err(err).into(),
};

[andrewt-amzn]

Why include the word Box in the type name? Isn't the type signature enough for the reader to know this is a Box, if that is important for them to know?

[marcbowes]

This has become idiomatic in libraries. I'm not sure what a better name might be, so I'm copying a pattern I've seen elsewhere. One example is the aws sdk itself.

[andrewt-amzn]

If the None variant is illegal, why use an Option at all?

[marcbowes]

This is a standard idiom with builders. Otherwise you need to make loads of builder structs to model a finite state machine:

struct QldbDriverBuilder;

impl QldbDriverBuilder {
  fn new() -> QldbDriverBuilder { QldbDriverBuilder {} }
  fn ledger_name(ledger_name: impl Into<String>) -> QldbDriverBuilderStep1 {
    QldbDriverBuilderStep1 { ledger_name: ledger_name.into() }
  }
}

struct QldbDriverBuilderStep1 {
  ledger_name: String
}

// ...

Because this isn't a "true runtime" error (i.e. something that randomly happens), it seems OK to allow for some misuse to escape the compiler.